svn commit: r346507 - in projects/fuse2/sys: kern sys

[Konstantin Belousov]

On Tue, Apr 23, 2019 at 08:50:21AM -0600, Alan Somers wrote:
> On Tue, Apr 23, 2019 at 6:47 AM Konstantin Belousov <kostikbel at gmail.com> wrote:
> >
> > On Mon, Apr 22, 2019 at 11:27:54AM -0600, Alan Somers wrote:
> > > On Mon, Apr 22, 2019 at 11:10 AM Konstantin Belousov
> > > <kostikbel at gmail.com> wrote:
> > > >
> > > > On Sun, Apr 21, 2019 at 11:04:06PM +0000, Alan Somers wrote:
> > > > > Author: asomers
> > > > > Date: Sun Apr 21 23:04:06 2019
> > > > > New Revision: 346507
> > > > > URL: https://svnweb.freebsd.org/changeset/base/346507
> > > > >
> > > > > Log:
> > > > >   fusefs: commit missing files from r346387
> > > > >
> > > > >   PR:         346357
> > > > >   Sponsored by:       The FreeBSD Foundation
> > > > >
> > > > > Modified:
> > > > >   projects/fuse2/sys/kern/kern_sig.c
> > > > >   projects/fuse2/sys/sys/signalvar.h
> > > > >
> > > > > Modified: projects/fuse2/sys/kern/kern_sig.c
> > > > > ==============================================================================
> > > > > --- projects/fuse2/sys/kern/kern_sig.c        Sun Apr 21 22:53:51 2019        (r346506)
> > > > > +++ projects/fuse2/sys/kern/kern_sig.c        Sun Apr 21 23:04:06 2019        (r346507)
> > > > > @@ -929,6 +929,23 @@ osigreturn(struct thread *td, struct osigreturn_args *
> > > > >  #endif
> > > > >  #endif /* COMPAT_43 */
> > > > >
> > > > > +/* Will this signal be fatal to the current process ? */
> > > > > +bool
> > > > > +sig_isfatal(struct proc *p, int sig)
> > > > > +{
> > > > > +     intptr_t act;
> > > > > +
> > > > > +     act = (intptr_t)p->p_sigacts->ps_sigact[_SIG_IDX(sig)];
> > > > > +     if ((intptr_t)SIG_DFL == act) {
> > > > > +             int prop;
> > > > This is against style.
> > > >
> > > > > +
> > > > > +             prop = sigprop(sig);
> > > > > +             return (0 != (prop & (SIGPROP_KILL | SIGPROP_CORE)));
> > > > > +     } else {
> > > > > +             return (false);
> > > > > +     }
> > > > > +}
> > > > Either your function lacks asserts about the owned locks, or it is racy.
> > >
> > > Good point.  I'll add lock assertions.
> > >
> > > >
> > > > Said that, is the comment above describes the intent ? The
> > > > implementation is too naive. Just for example, blocked signals with
> > > > default disposition do not result in the termination. On the other hand,
> > > > blocked ignored traps cause immediate termination.
> > >
> > > I'm using this in a context where the signal has already been
> > > delivered and caught.  So it can't be blocked, and it can't be a trap.
> > >
> > > >
> > > > Overall, I do not believe that it is possible to implement that without
> > > > duplicating the code of tdsendsignal() and trapsignal(), i.e. you should
> > > > additionally provide the originating context, besides a signal number.
> > >
> > > Do you still believe that even though it doesn't need to consider
> > > blocked signals and traps?
> > Generally yes, but lets see the specifics of the use.
> >
> > >
> > > >
> > > > What you are trying to do there ?
> > >
> > > It's in a situation where a syscall can't simply return EINTR or
> > > ERESTART.  I need to do some extra work to interrupt the syscall (ask
> > > the FUSE daemon to interrupt the associated FUSE operation).  If the
> > > signal will be fatal, then there's no point in waiting for the FUSE
> > > daemon to reply and I can simply return EINTR.  However, if the signal
> > > is not fatal, then I need to wait to see if the FUSE daemon to
> > > acknowledge the interrupt or else complete the operation like normal.
> > In what context does the interruption happen ?  Is it for a thread of the
> > fuse daemon, or normal process ?
> 
> It's usually a normal process, but it could be another kernel thread,
> like aiod.  It will never be the fuse daemon.
> 
> >
> > Can you point out the specific fragment of code where the function is used ?
> 
> Here's the function that uses it:
> https://svnweb.freebsd.org/base/projects/fuse2/sys/fs/fuse/fuse_ipc.c?view=markup#l429

This is even more confusing.

I do not understand the kern_sigprocmask() calls around msleep(9) with
empty SIG_BLOCK mask, this looks like a nop.  What is the purpose ?

Then, calling cursig() has much more implications than just returning
the lowest pending signal number, for instance it redirects the signal
to debugger, or reacts to SIGSTOP. Do you have any resources owned at
this time, most likely a vnode lock ?

What if another thread in the mt process terminates the process, and now
waits for this one to reach the safe boundary ?

Overall, it is not a filesystem business to even look into the signal
state of the process.  You should react to EINTR/ERESTART from msleep(9)
at most.

NFS approach, although not very good, is definitely better than this.
NFS provides intr mount option vs. hard (default) mounts.   If intr
option is specified, then VOPs sleep with PCATCH, and pass EINTR/ERESTART
up, orphaning the request.  For hard mounts, PCATCH is not used.

You would need to complete in-flight fuse daemon requests even if orphaned
by the requested thread due to the interruption.

There is very delicate TDF_SBDRY mechanism which defers stopping of the
sleeping threads which owns resources like vnode locks, and which controls 
controls the reaction to the pending signal (selection of EINTR vs ERESTART,
which is important for advisory locks because they must not auto-restart).
It is used by NFS to avoid situation where the thread inside msleep(PCATCH)
is stopped by SIGSTOP, and then vnode lock cascade kills all filesystem
accesses.  See sigdeferstop()/sigallowstop().