On Tue, Jun 07, 2016 at 07:01:55PM +0300, Konstantin Belousov wrote: > On Tue, Jun 07, 2016 at 04:24:53PM +0200, Jilles Tjoelker wrote: > > On Tue, Jun 07, 2016 at 07:29:56AM +0300, Konstantin Belousov wrote: > > > This looks as if we should not ignore suspension requests in > > > thread_suspend_check() completely in TDF_SBDRY case, but return either > > > EINTR or ERESTART (most likely ERESTART). Note that the goal of > > > TDF_SBDRY is to avoid suspending in the protected region, not to make an > > > impression that the suspension does not occur at all.
> > This looks like it would revert r246417 and re-introduce the bug fixed > > by it (unexpected [EINTR] and short reads/writes after stop signals). > Well, the patch returns ERESTART and not EINTR, so the syscall should > be retried after all the unwinding. That fixes the [EINTR] part of the problem but not short reads and writes. > > After r246417, TDF_SBDRY is intended for sleeps that occur while holding > > resources such as vnode locks and are normally short but should be > > interruptible by fatal signals because they may occasionally be > > indefinitely long (such as a non-responsive NFS server). > > It looks like yet another kind of sleep may be required, since advisory > > locks still hold some filesystem resources across the sleep (though not > > vnode locks). > I do not think that adv locks enter sleep with any resource held which > would block other threads. But I agree with the statement because the > lock might be granted and then the stopped thread would appear to own > the blocking resource. It does not hold any resource used by normal operations, but it blocks a forced unmount (umount -f hangs in [purgelocks] with tmpfs in a recent stable/10). If queuing is supposed to be fair, then granting the lock to the stopped thread is correct and aborting the sleep with [ERESTART] would break it. The kern_lockf.c code seems to go to some lengths to make queuing fair. This does not seem very important, though. Also, restarting a locking call violates some text in POSIX XSH's fcntl page that the range of bytes to be locked shall be determined before the thread blocks (this may be affected by the current seek offset and the file size). I don't know whether violating this will break any applications. The text has the problem that there is no way to distinguish between a thread that is in fcntl() and has not yet blocked and a thread that has blocked, even though it seems intuitively clear. > > We then have four kinds: > > * uninterruptible by signals, ignores stops (default) > > * interruptible by signals, ignores stops (current TDF_SBDRY with > > PCATCH) > > * interruptible by signals, freezes in place on stops (avoids > > unexpected short I/O) (current PCATCH, otherwise) > > * interruptible by signals, fails with [ERESTART] on stops (avoids > > holding resources across a stop) (new) > > The new kind of sleep would fail with [ERESTART] only for stops, since > > [EINTR] should only be returned if a signal handler was called. There > > cannot be a signal handler since a SIGTSTP/SIGTTIN/SIGTTOU signal with a > > handler does not stop the process. > And where would this new kind of sleep used ? The advlock sleep is the one > place. Does fifo sleep for reader or writer on open require this kind > of handling (IMO no) ? > I think this can be relatively easily implemented with either a flag > for XXXsleep(9) (my older style of PBDRY) or using only the thread flag > (jhb' newer TDF_SBDRY approach). Probably the later should be used, for > consistency and easier marking of larger blocks of code. In this case it is clear which sleep(9) calls should be affected so it may be better to avoid more hidden state. I also wonder whether we may be overengineering things here. Perhaps the advlock sleep can simply turn off TDF_SBDRY. -- Jilles Tjoelker _______________________________________________ email@example.com mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"