Re: thread suspension when dumping core

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

Like this.

diff --git a/sys/kern/kern_lockf.c b/sys/kern/kern_lockf.c
index a0a3789..ee26596 100644
--- a/sys/kern/kern_lockf.c
+++ b/sys/kern/kern_lockf.c
_at__at_ -1378,7 +1378,7 _at__at_ lf_setlock(struct lockf *state, struct lockf_entry *lock, struct vnode *vp,
     void **cookiep)
 {
 	static char lockstr[] = "lockf";
-	int priority, error;
+	int error, priority, stoprestart;
 
 #ifdef LOCKF_DEBUG
 	if (lockf_debug & 1)
_at__at_ -1466,7 +1466,10 _at__at_ lf_setlock(struct lockf *state, struct lockf_entry *lock, struct vnode *vp,
 		}
 
 		lock->lf_refs++;
+		stoprestart = sigstoprestart();
 		error = sx_sleep(lock, &state->ls_lock, priority, lockstr, 0);
+		if (stoprestart)
+			sigstopnormal();
 		if (lf_free_lock(lock)) {
 			error = EDOOFUS;
 			goto out;
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 75a1259..1d7036d 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
_at__at_ -2633,6 +2633,35 _at__at_ sigallowstop(void)
 	return (prev);
 }
 
+int
+sigstoprestart(void)
+{
+	struct thread *td;
+
+	td = curthread;
+	if ((td->td_flags & TDF_SBDRY) == 0 ||
+	    (td->td_flags & TDF_SRESTART) != 0)
+		return (0);
+	thread_lock(td);
+	td->td_flags |= TDF_SRESTART;
+	thread_unlock(td);
+	return (1);
+}
+
+int
+sigstopnormal(void)
+{
+	struct thread *td;
+	int prev;
+
+	td = curthread;
+	thread_lock(td);
+	prev = (td->td_flags & TDF_SRESTART) != 0;
+	td->td_flags &= ~TDF_SRESTART;
+	thread_unlock(td);
+	return (prev);
+}
+
 /*
  * If the current process has received a signal (should be caught or cause
  * termination, should interrupt current syscall), return the signal number.
diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c
index 9af377e..6460ae9 100644
--- a/sys/kern/kern_thread.c
+++ b/sys/kern/kern_thread.c
_at__at_ -932,7 +932,8 _at__at_ thread_suspend_check(int return_instead)
 		if ((td->td_flags & TDF_SBDRY) != 0) {
 			KASSERT(return_instead,
 			    ("TDF_SBDRY set for unsafe thread_suspend_check"));
-			return (0);
+			return ((td->td_flags & TDF_SRESTART) != 0 ?
+			    ERESTART : 0);
 		}
 
 		/*
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 629f7e8..1e986a9 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
_at__at_ -395,7 +395,7 _at__at_ do {									\
 #define	TDF_NEEDRESCHED	0x00010000 /* Thread needs to yield. */
 #define	TDF_NEEDSIGCHK	0x00020000 /* Thread may need signal delivery. */
 #define	TDF_NOLOAD	0x00040000 /* Ignore during load avg calculations. */
-#define	TDF_UNUSED19	0x00080000 /* --available-- */
+#define	TDF_SRESTART	0x00080000 /* ERESTART on stop attempts. */
 #define	TDF_THRWAKEUP	0x00100000 /* Libthr thread must not suspend itself. */
 #define	TDF_UNUSED21	0x00200000 /* --available-- */
 #define	TDF_SWAPINREQ	0x00400000 /* Swapin request due to wakeup. */
diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h
index e574ec3..3d4c4a5 100644
--- a/sys/sys/signalvar.h
+++ b/sys/sys/signalvar.h
_at__at_ -328,6 +328,8 _at__at_ extern struct mtx	sigio_lock;
 int	cursig(struct thread *td);
 int	sigdeferstop(void);
 int	sigallowstop(void);
+int	sigstoprestart(void);
+int	sigstopnormal(void);
 void	execsigs(struct proc *p);
 void	gsignal(int pgid, int sig, ksiginfo_t *ksi);
 void	killproc(struct proc *p, char *why);