Message-ID: <20080630083536.680b093a@barsoom.rdu.redhat.com>
Date:	Mon, 30 Jun 2008 08:35:36 -0400
From:	Jeff Layton <jlayton@...hat.com>
To:	Neil Brown <neilb@...e.de>
Cc:	"J. Bruce Fields" <bfields@...ldses.org>,
	linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH - take 2] knfsd: nfsd: Handle ERESTARTSYS from  
 syscalls. (and possible kthread_stop changes)

On Tue, 24 Jun 2008 09:55:55 +1000
Neil Brown <neilb@...e.de> wrote:

> On Sunday June 22, jlayton@...hat.com wrote:
> > On Mon, 23 Jun 2008 10:20:59 +1000
> > Neil Brown <neilb@...e.de> wrote:
> > > 
> > > I think my leaning is just to remove the distinction.  About the worst
> > > that can happen if the filesystem decides to respond to the signal is
> > > that you get a short write or a short read, both of which should be
> > > correctly handled by the client.
> > > 
> > 
> > Good points all around. If the sigmask juggling makes no sense, then
> > I'm OK with removing it. I'd prefer that we simplify the code rather
> > than trying to get clever with init scripts anyway...
> > 
> 
> Let's do it?
> 

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.

------------[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@...hat.com>
---
 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, &current.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@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists