Re: thread suspension when dumping core

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.

> 
> 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.

> 
> 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.