On Mon, Jun 30, 2008 at 08:35:36AM -0400, Jeff Layton wrote:
> The idea looks reasonable, but the patch above doesn't apply to Bruce's
> for-2.6.27 tree. The nfsd() function has changed a bit. Actually, I
> think we can make this even simpler. kthreads start with all signals
> ignored and the new patch enables all of the ones in SHUTDOWN_SIGS. So
> I don't think we need any signal masking at all.
>
> How about this patch? It should apply on top of the patchset already
> in Bruce's tree.
Looks reasonable to me; applied for 2.6.27 barring any test failures or
objections.
--b.
>
> ------------[snip]--------------
>
> Subject: [PATCH] knfsd: treat all shutdown signals as equivalent
>
> knfsd currently uses 2 signal masks when processing requests. A "loose"
> mask (SHUTDOWN_SIGS) that it uses when receiving network requests, and
> then a more "strict" mask (ALLOWED_SIGS, which is just SIGKILL) that it
> allows when doing the actual operation on the local storage.
>
> This is apparently unnecessarily complicated. The underlying filesystem
> should be able to sanely handle a signal in the middle of an operation.
> This patch removes the signal mask handling from knfsd altogether. When
> knfsd is started as a kthread, all signals are ignored. It then allows
> all of the signals in SHUTDOWN_SIGS. There's no need to set the mask
> as well.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/nfsd/nfssvc.c | 20 ++------------------
> 1 files changed, 2 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 96fdbca..d20a087 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -37,13 +37,7 @@
>
> #define NFSDDBG_FACILITY NFSDDBG_SVC
>
> -/* these signals will be delivered to an nfsd thread
> - * when handling a request
> - */
> -#define ALLOWED_SIGS (sigmask(SIGKILL))
> -/* these signals will be delivered to an nfsd thread
> - * when not handling a request. i.e. when waiting
> - */
> +/* These signals can be used to shutdown an nfsd thread. */
> #define SHUTDOWN_SIGS (sigmask(SIGKILL) | sigmask(SIGHUP) | sigmask(SIGINT) | sigmask(SIGQUIT))
>
> extern struct svc_program nfsd_program;
> @@ -414,7 +408,6 @@ nfsd(void *vrqstp)
> {
> struct svc_rqst *rqstp = (struct svc_rqst *) vrqstp;
> struct fs_struct *fsp;
> - sigset_t shutdown_mask, allowed_mask;
> int err, preverr = 0;
> unsigned int signo;
>
> @@ -433,15 +426,12 @@ nfsd(void *vrqstp)
> current->fs = fsp;
> current->fs->umask = 0;
>
> - siginitsetinv(&shutdown_mask, SHUTDOWN_SIGS);
> - siginitsetinv(&allowed_mask, ALLOWED_SIGS);
> -
> /*
> * thread is spawned with all signals set to SIG_IGN, re-enable
> * the ones that matter
> */
> for (signo = 1; signo <= _NSIG; signo++) {
> - if (!sigismember(&shutdown_mask, signo))
> + if (siginmask(signo, SHUTDOWN_SIGS))
> allow_signal(signo);
> }
>
> @@ -460,9 +450,6 @@ nfsd(void *vrqstp)
> * The main request loop
> */
> for (;;) {
> - /* Block all but the shutdown signals */
> - sigprocmask(SIG_SETMASK, &shutdown_mask, NULL);
> -
> /*
> * Find a socket with data available and call its
> * recvfrom routine.
> @@ -487,9 +474,6 @@ nfsd(void *vrqstp)
> /* Lock the export hash tables for reading. */
> exp_readlock();
>
> - /* Process request with signals blocked. */
> - sigprocmask(SIG_SETMASK, &allowed_mask, NULL);
> -
> svc_process(rqstp);
>
> /* Unlock export hash tables */
> --
> 1.5.5.1
>
>
>
> > >
> > > The latest patch doesn't use kthread_stop with nfsd. I figured it was
> > > best to avoid having multiple takedown methods, and since we're using
> > > signals here anyway, I just kept signaling as the only way to take down
> > > nfsd. Plus, as you mention -- all kthread_stop's are serialized, which
> > > could have meant that nfsd's would take a long time to come down on a
> > > busy box with a lot of them.
> >
> > Oh, good. I must have been looking at an old patch.
> >
> > >
> > > As a side note, I'm not terribly thrilled with how kthread_stop is
> > > implemented. I worry that a stuck kthread would block unrelated
> > > kthreads from coming down. I did a patch 6 mos ago or so to make it so
> > > that kthread_stop's could be done in parallel. The downside was that it
> > > added a new field to the task_struct (which is already too bloated,
> > > IMO). It might be worth resurrecting that at some point (possibly
> > > making the new field a union with another field that's unused in
> > > kthreads), or considering a different approach to parallelize
> > > kthread_stop.
> >
> > One the one hand, kthreads are expected to not block and to check for
> > kthread_should_stop very often, so this wouldn't be a problem.
> > On the other hand, expectations like this are quickly invalidated, and
> > the code probably should be fixed.
> > I suspect we could do without adding anything to task_struct.
> > Instead of just one kthread_stop_info, have a linked list, one entry
> > for each thread being stopped at the moment. kthread_stop adds to
> > the list, sets PF_EXITING (I hope that is safe) and wakes the process.
> > kthread_should_stop tests PF_EXITING.
> > When kthread() finishes, it does a linear search (the list should be
> > short) and calls complete and the relevant .done.
> >
> > Something like this?
> >
> > NeilBrown
> >
> >
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index bd1b9ea..107290a 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -39,12 +39,13 @@ struct kthread_stop_info
> > struct task_struct *k;
> > int err;
> > struct completion done;
> > + struct kthread_stp_info *next;
> > };
> >
> > /* Thread stopping is done by setthing this var: lock serializes
> > * multiple kthread_stop calls. */
> > static DEFINE_MUTEX(kthread_stop_lock);
> > -static struct kthread_stop_info kthread_stop_info;
> > +static struct kthread_stop_info *kthread_stop_info;
> >
> > /**
> > * kthread_should_stop - should this kthread return now?
> > @@ -55,7 +56,7 @@ static struct kthread_stop_info kthread_stop_info;
> > */
> > int kthread_should_stop(void)
> > {
> > - return (kthread_stop_info.k == current);
> > + return test_bit(PF_EXITING, ¤t.flags);
> > }
> > EXPORT_SYMBOL(kthread_should_stop);
> >
> > @@ -80,8 +81,17 @@ static int kthread(void *_create)
> >
> > /* It might have exited on its own, w/o kthread_stop. Check. */
> > if (kthread_should_stop()) {
> > - kthread_stop_info.err = ret;
> > - complete(&kthread_stop_info.done);
> > + struct kthread_stop_info **ksip;
> > + mutex_lock(&kthread_stop_lock);
> > + for (ksip = &kthread_stop_info ; *ksip ; ksip = &(*ksip)->next)
> > + if ((*ksip)->k == current)
> > + break;
> > + *ksip = (*ksip)->next;
> > + (*ksip)->next = NULL;
> > + mutex_unlock(&kthread_stop_lock);
> > +
> > + ksi->err = ret;
> > + complete(&ksi->done);
> > }
> > return 0;
> > }
> > @@ -199,26 +209,20 @@ EXPORT_SYMBOL(kthread_bind);
> > int kthread_stop(struct task_struct *k)
> > {
> > int ret;
> > + struct kthread_stop_info ksi;
> >
> > mutex_lock(&kthread_stop_lock);
> >
> > - /* It could exit after stop_info.k set, but before wake_up_process. */
> > - get_task_struct(k);
> > + init_completion(&ksi->done);
> > + ksi->k = k;
> >
> > - /* Must init completion *before* thread sees kthread_stop_info.k */
> > - init_completion(&kthread_stop_info.done);
> > - smp_wmb();
> > -
> > - /* Now set kthread_should_stop() to true, and wake it up. */
> > - kthread_stop_info.k = k;
> > + set_bit(PF_EXITING, &k->flags);
> > wake_up_process(k);
> > - put_task_struct(k);
> > + mutex_unlock(&kthread_stop_lock);
> >
> > - /* Once it dies, reset stop ptr, gather result and we're done. */
> > - wait_for_completion(&kthread_stop_info.done);
> > - kthread_stop_info.k = NULL;
> > + /* Once it dies gather result and we're done. */
> > + wait_for_completion(&ksi->done);
> > ret = kthread_stop_info.err;
> > - mutex_unlock(&kthread_stop_lock);
> >
> > return ret;
> > }
>
>
> That looks reasonable to me, though I tend to find standard list
> functions easier to follow...
>
> The main reason I was going to add a field to the task_struct was
> because I wanted to make sure that kthread_should_stop() was a quick
> operation. If we can just use PF_EXITING then that's definitely a
> better scheme.
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
