[PATCH 0/3] NLM: convert lockd to kthreads (try #11)

[PATCH 0/3] NLM: convert lockd to kthreads (try #11)

[Jeff Layton]

This is try #11 of the conversion of lockd to the kthread API.

The main differences from the last patchset are:

1) the addition of a patch to svc_recv to have it check
   kthread_should_stop in various places

2) the removal of the send_sig() call from lockd_down

...as Trond cleverly pointed out, if we strategically place a
kthread_should_stop call after changing the task state, then we don't
need anything else to prevent a long hang in schedule_timeout().

Comments, suggestions or ACK's appreciated...

Thanks,

Jeff Layton <jlayton@redhat.com>

[PATCH 1/3] SUNRPC: export svc_sock_update_bufs

[Jeff Layton]

Needed since the plan is to not have a svc_create_thread helper and to
have current users of that function just call kthread_run directly.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Reviewed-by: NeilBrown <neilb@suse.de>
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 net/sunrpc/svcsock.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 1d3e5fc..b73a92a 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1101,6 +1101,7 @@ void svc_sock_update_bufs(struct svc_serv *serv)
 	}
 	spin_unlock_bh(&serv->sv_lock);
 }
+EXPORT_SYMBOL(svc_sock_update_bufs);
 
 /*
  * Initialize socket for RPC use and create svc_sock struct
-- 
1.5.3.8

[PATCH 2/3] SUNRPC: have svc_recv() check kthread_should_stop()

[Jeff Layton]

When using kthreads that call into svc_recv, we want to make sure that
they do not block there for a long time when we're trying to take down
the kthread.

This patch changes svc_recv() to check kthread_should_stop() at the same
places that it checks to see if it's signalled(). Also check just before
svc_recv() tries to schedule(). By making sure that we check it just
after setting the task state we can avoid having to use any locking or
signalling to ensure it doesn't block for a long time.

There's still a chance of a 500ms sleep if alloc_page() fails, but
that should be a rare occurrence and isn't a terribly long time in
the context of a kthread being taken down.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/svc_xprt.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index ea377e0..a3165a2 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -18,6 +18,7 @@
 #include <linux/skbuff.h>
 #include <linux/file.h>
 #include <linux/freezer.h>
+#include <linux/kthread.h>
 #include <net/sock.h>
 #include <net/checksum.h>
 #include <net/ip.h>
@@ -586,6 +587,8 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 		while (rqstp->rq_pages[i] == NULL) {
 			struct page *p = alloc_page(GFP_KERNEL);
 			if (!p) {
+				if (kthread_should_stop())
+					return -EINTR;
 				int j = msecs_to_jiffies(500);
 				schedule_timeout_uninterruptible(j);
 			}
@@ -607,7 +610,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 
 	try_to_freeze();
 	cond_resched();
-	if (signalled())
+	if (signalled() || kthread_should_stop())
 		return -EINTR;
 
 	spin_lock_bh(&pool->sp_lock);
@@ -626,6 +629,20 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 		 * to bring down the daemons ...
 		 */
 		set_current_state(TASK_INTERRUPTIBLE);
+
+		/*
+		 * checking kthread_should_stop() here allows us to avoid
+		 * locking and signalling when stopping kthreads that call
+		 * svc_recv. If the thread has already been woken up, then
+		 * we can exit here without sleeping. If not, then it
+		 * it'll be woken up quickly during the schedule_timeout
+		 */
+		if (kthread_should_stop()) {
+			set_current_state(TASK_RUNNING);
+			spin_unlock_bh(&pool->sp_lock);
+			return -EINTR;
+		}
+
 		add_wait_queue(&rqstp->rq_wait, &wait);
 		spin_unlock_bh(&pool->sp_lock);
 
@@ -641,7 +658,10 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 			svc_thread_dequeue(pool, rqstp);
 			spin_unlock_bh(&pool->sp_lock);
 			dprintk("svc: server %p, no data yet\n", rqstp);
-			return signalled()? -EINTR : -EAGAIN;
+			if (signalled() || kthread_should_stop())
+				return -EINTR;
+			else
+				return -EAGAIN;
 		}
 	}
 	spin_unlock_bh(&pool->sp_lock);
-- 
1.5.3.8

[PATCH 3/3] NLM: Convert lockd to use kthreads

[Jeff Layton]

Have lockd_up start lockd using kthread_run. With this change,
lockd_down now blocks until lockd actually exits, so there's no longer
need for the waitqueue code at the end of lockd_down. This also means
that only one lockd can be running at a time which simplifies the code
within lockd's main loop.

This also adds a check for kthread_should_stop in the main loop of
nlmsvc_retry_blocked and after that function returns. There's no sense
continuing to retry blocks if lockd is coming down anyway.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/lockd/svc.c     |  136 ++++++++++++++++++++++++----------------------------
 fs/lockd/svclock.c |    3 +-
 2 files changed, 64 insertions(+), 75 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 0822646..378b393 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -25,6 +25,7 @@
 #include <linux/smp.h>
 #include <linux/smp_lock.h>
 #include <linux/mutex.h>
+#include <linux/kthread.h>
 #include <linux/freezer.h>
 
 #include <linux/sunrpc/types.h>
@@ -48,14 +49,11 @@ EXPORT_SYMBOL(nlmsvc_ops);
 
 static DEFINE_MUTEX(nlmsvc_mutex);
 static unsigned int		nlmsvc_users;
-static pid_t			nlmsvc_pid;
+static struct task_struct	*nlmsvc_task;
 static struct svc_serv		*nlmsvc_serv;
 int				nlmsvc_grace_period;
 unsigned long			nlmsvc_timeout;
 
-static DECLARE_COMPLETION(lockd_start_done);
-static DECLARE_WAIT_QUEUE_HEAD(lockd_exit);
-
 /*
  * These can be set at insmod time (useful for NFS as root filesystem),
  * and also changed through the sysctl interface.  -- Jamie Lokier, Aug 2003
@@ -111,35 +109,30 @@ static inline void clear_grace_period(void)
 /*
  * This is the lockd kernel thread
  */
-static void
-lockd(struct svc_rqst *rqstp)
+static int
+lockd(void *vrqstp)
 {
 	int		err = 0;
+	struct svc_rqst *rqstp = vrqstp;
 	unsigned long grace_period_expire;
 
-	/* Lock module and set up kernel thread */
-	/* lockd_up is waiting for us to startup, so will
-	 * be holding a reference to this module, so it
-	 * is safe to just claim another reference
-	 */
-	__module_get(THIS_MODULE);
-	lock_kernel();
-
-	/*
-	 * Let our maker know we're running.
-	 */
-	nlmsvc_pid = current->pid;
-	nlmsvc_serv = rqstp->rq_server;
-	complete(&lockd_start_done);
-
-	daemonize("lockd");
+	/* try_to_freeze() is called from svc_recv() */
 	set_freezable();
 
-	/* Process request with signals blocked, but allow SIGKILL.  */
+	/* Allow SIGKILL to tell lockd to drop all of its locks */
 	allow_signal(SIGKILL);
 
 	dprintk("NFS locking service started (ver " LOCKD_VERSION ").\n");
 
+	/*
+	 * FIXME: it would be nice if lockd didn't spend its entire life
+	 * running under the BKL. At the very least, it would be good to
+	 * have someone clarify what it's intended to protect here. I've
+	 * seen some handwavy posts about posix locking needing to be
+	 * done under the BKL, but it's far from clear.
+	 */
+	lock_kernel();
+
 	if (!nlm_timeout)
 		nlm_timeout = LOCKD_DFLT_TIMEO;
 	nlmsvc_timeout = nlm_timeout * HZ;
@@ -148,10 +141,9 @@ lockd(struct svc_rqst *rqstp)
 
 	/*
 	 * The main request loop. We don't terminate until the last
-	 * NFS mount or NFS daemon has gone away, and we've been sent a
-	 * signal, or else another process has taken over our job.
+	 * NFS mount or NFS daemon has gone away.
 	 */
-	while ((nlmsvc_users || !signalled()) && nlmsvc_pid == current->pid) {
+	while (!kthread_should_stop()) {
 		long timeout = MAX_SCHEDULE_TIMEOUT;
 		char buf[RPC_MAX_ADDRBUFLEN];
 
@@ -161,6 +153,7 @@ lockd(struct svc_rqst *rqstp)
 				nlmsvc_invalidate_all();
 				grace_period_expire = set_grace_period();
 			}
+			continue;
 		}
 
 		/*
@@ -174,6 +167,10 @@ lockd(struct svc_rqst *rqstp)
 		} else if (time_before(grace_period_expire, jiffies))
 			clear_grace_period();
 
+		/* nlmsvc_retry_blocked can block, so check for kthread_stop */
+		if (kthread_should_stop())
+			break;
+
 		/*
 		 * Find a socket with data available and call its
 		 * recvfrom routine.
@@ -195,28 +192,19 @@ lockd(struct svc_rqst *rqstp)
 	}
 
 	flush_signals(current);
+	if (nlmsvc_ops)
+		nlmsvc_invalidate_all();
+	nlm_shutdown_hosts();
 
-	/*
-	 * Check whether there's a new lockd process before
-	 * shutting down the hosts and clearing the slot.
-	 */
-	if (!nlmsvc_pid || current->pid == nlmsvc_pid) {
-		if (nlmsvc_ops)
-			nlmsvc_invalidate_all();
-		nlm_shutdown_hosts();
-		nlmsvc_pid = 0;
-		nlmsvc_serv = NULL;
-	} else
-		printk(KERN_DEBUG
-			"lockd: new process, skipping host shutdown\n");
-	wake_up(&lockd_exit);
+	unlock_kernel();
+
+	nlmsvc_task = NULL;
+	nlmsvc_serv = NULL;
 
 	/* Exit the RPC thread */
 	svc_exit_thread(rqstp);
 
-	/* Release module */
-	unlock_kernel();
-	module_put_and_exit(0);
+	return 0;
 }
 
 /*
@@ -261,14 +249,15 @@ static int make_socks(struct svc_serv *serv, int proto)
 int
 lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
 {
-	struct svc_serv *	serv;
-	int			error = 0;
+	struct svc_serv *serv;
+	struct svc_rqst *rqstp;
+	int		error = 0;
 
 	mutex_lock(&nlmsvc_mutex);
 	/*
 	 * Check whether we're already up and running.
 	 */
-	if (nlmsvc_pid) {
+	if (nlmsvc_serv) {
 		if (proto)
 			error = make_socks(nlmsvc_serv, proto);
 		goto out;
@@ -295,13 +284,28 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
 	/*
 	 * Create the kernel thread and wait for it to start.
 	 */
-	error = svc_create_thread(lockd, serv);
-	if (error) {
+	rqstp = svc_prepare_thread(serv, &serv->sv_pools[0]);
+	if (IS_ERR(rqstp)) {
+		error = PTR_ERR(rqstp);
+		printk(KERN_WARNING
+			"lockd_up: svc_rqst allocation failed, error=%d\n",
+			error);
+		goto destroy_and_out;
+	}
+
+	svc_sock_update_bufs(serv);
+	nlmsvc_serv = rqstp->rq_server;
+
+	nlmsvc_task = kthread_run(lockd, rqstp, serv->sv_name);
+	if (IS_ERR(nlmsvc_task)) {
+		error = PTR_ERR(nlmsvc_task);
+		nlmsvc_task = NULL;
+		nlmsvc_serv = NULL;
 		printk(KERN_WARNING
-			"lockd_up: create thread failed, error=%d\n", error);
+			"lockd_up: kthread_run failed, error=%d\n", error);
+		svc_exit_thread(rqstp);
 		goto destroy_and_out;
 	}
-	wait_for_completion(&lockd_start_done);
 
 	/*
 	 * Note: svc_serv structures have an initial use count of 1,
@@ -323,37 +327,21 @@ EXPORT_SYMBOL(lockd_up);
 void
 lockd_down(void)
 {
-	static int warned;
-
 	mutex_lock(&nlmsvc_mutex);
 	if (nlmsvc_users) {
 		if (--nlmsvc_users)
 			goto out;
-	} else
-		printk(KERN_WARNING "lockd_down: no users! pid=%d\n", nlmsvc_pid);
-
-	if (!nlmsvc_pid) {
-		if (warned++ == 0)
-			printk(KERN_WARNING "lockd_down: no lockd running.\n"); 
-		goto out;
+	} else {
+		printk(KERN_ERR "lockd_down: no users! task=%p\n",
+			nlmsvc_task);
+		BUG();
 	}
-	warned = 0;
 
-	kill_proc(nlmsvc_pid, SIGKILL, 1);
-	/*
-	 * Wait for the lockd process to exit, but since we're holding
-	 * the lockd semaphore, we can't wait around forever ...
-	 */
-	clear_thread_flag(TIF_SIGPENDING);
-	interruptible_sleep_on_timeout(&lockd_exit, HZ);
-	if (nlmsvc_pid) {
-		printk(KERN_WARNING 
-			"lockd_down: lockd failed to exit, clearing pid\n");
-		nlmsvc_pid = 0;
+	if (!nlmsvc_task) {
+		printk(KERN_ERR "lockd_down: no lockd running.\n");
+		BUG();
 	}
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	kthread_stop(nlmsvc_task);
 out:
 	mutex_unlock(&nlmsvc_mutex);
 }
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 2f4d8fa..d8324b6 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -29,6 +29,7 @@
 #include <linux/sunrpc/svc.h>
 #include <linux/lockd/nlm.h>
 #include <linux/lockd/lockd.h>
+#include <linux/kthread.h>
 
 #define NLMDBG_FACILITY		NLMDBG_SVCLOCK
 
@@ -867,7 +868,7 @@ nlmsvc_retry_blocked(void)
 	unsigned long	timeout = MAX_SCHEDULE_TIMEOUT;
 	struct nlm_block *block;
 
-	while (!list_empty(&nlm_blocked)) {
+	while (!list_empty(&nlm_blocked) && !kthread_should_stop()) {
 		block = list_entry(nlm_blocked.next, struct nlm_block, b_list);
 
 		if (block->b_when == NLM_NEVER)
-- 
1.5.3.8

Re: [PATCH 2/3] SUNRPC: have svc_recv() check kthread_should_stop()

[J. Bruce Fields]

Another nit:

On Thu, Feb 07, 2008 at 04:34:54PM -0500, Jeff Layton wrote:
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index ea377e0..a3165a2 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -18,6 +18,7 @@
>  #include <linux/skbuff.h>
>  #include <linux/file.h>
>  #include <linux/freezer.h>
> +#include <linux/kthread.h>
>  #include <net/sock.h>
>  #include <net/checksum.h>
>  #include <net/ip.h>
> @@ -586,6 +587,8 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
>  		while (rqstp->rq_pages[i] == NULL) {
>  			struct page *p = alloc_page(GFP_KERNEL);
>  			if (!p) {
> +				if (kthread_should_stop())
> +					return -EINTR;
>  				int j = msecs_to_jiffies(500);
>  				schedule_timeout_uninterruptible(j);
>  			}

The compiler's whining about the mixed declarations and code, so I've
also done the following in my copy.

--b.

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index a3165a2..53c8ea9 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -587,9 +587,9 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 		while (rqstp->rq_pages[i] == NULL) {
 			struct page *p = alloc_page(GFP_KERNEL);
 			if (!p) {
+				int j = msecs_to_jiffies(500);
 				if (kthread_should_stop())
 					return -EINTR;
-				int j = msecs_to_jiffies(500);
 				schedule_timeout_uninterruptible(j);
 			}
 			rqstp->rq_pages[i] = p;

Re: [PATCH 2/3] SUNRPC: have svc_recv() check kthread_should_stop()

[Jeff Layton]

On Sun, 10 Feb 2008 19:47:59 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> Another nit:
> 
> On Thu, Feb 07, 2008 at 04:34:54PM -0500, Jeff Layton wrote:
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index ea377e0..a3165a2 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/skbuff.h>
> >  #include <linux/file.h>
> >  #include <linux/freezer.h>
> > +#include <linux/kthread.h>
> >  #include <net/sock.h>
> >  #include <net/checksum.h>
> >  #include <net/ip.h>
> > @@ -586,6 +587,8 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
> >  		while (rqstp->rq_pages[i] == NULL) {
> >  			struct page *p = alloc_page(GFP_KERNEL);
> >  			if (!p) {
> > +				if (kthread_should_stop())
> > +					return -EINTR;
> >  				int j = msecs_to_jiffies(500);
> >  				schedule_timeout_uninterruptible(j);
> >  			}
> 
> The compiler's whining about the mixed declarations and code, so I've
> also done the following in my copy.
> 
> --b.
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index a3165a2..53c8ea9 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -587,9 +587,9 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
>  		while (rqstp->rq_pages[i] == NULL) {
>  			struct page *p = alloc_page(GFP_KERNEL);
>  			if (!p) {
> +				int j = msecs_to_jiffies(500);
>  				if (kthread_should_stop())
>  					return -EINTR;
> -				int j = msecs_to_jiffies(500);
>  				schedule_timeout_uninterruptible(j);
>  			}
>  			rqstp->rq_pages[i] = p;

Thanks Bruce. This reminds me though that I had a question about the
above. I'm assuming that it's ok to return -EINTR without allocating
all of the pages that we'll need to actually do the recv. This seems to
be OK for all of the in-kernel callers of svc_recv...

Given that, is there any reason we need an uninterruptible sleep there?
It looks like when under heavy memory pressure, svc_recv could loop there
for a long time. Allowing it to exit from that loop when signalled seems
like it would be a good thing...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/3] SUNRPC: have svc_recv() check kthread_should_stop()

[J. Bruce Fields]

On Mon, Feb 11, 2008 at 07:06:06AM -0500, Jeff Layton wrote:
> On Sun, 10 Feb 2008 19:47:59 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > Another nit:
> > 
> > On Thu, Feb 07, 2008 at 04:34:54PM -0500, Jeff Layton wrote:
> > > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > > index ea377e0..a3165a2 100644
> > > --- a/net/sunrpc/svc_xprt.c
> > > +++ b/net/sunrpc/svc_xprt.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/skbuff.h>
> > >  #include <linux/file.h>
> > >  #include <linux/freezer.h>
> > > +#include <linux/kthread.h>
> > >  #include <net/sock.h>
> > >  #include <net/checksum.h>
> > >  #include <net/ip.h>
> > > @@ -586,6 +587,8 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
> > >  		while (rqstp->rq_pages[i] == NULL) {
> > >  			struct page *p = alloc_page(GFP_KERNEL);
> > >  			if (!p) {
> > > +				if (kthread_should_stop())
> > > +					return -EINTR;
> > >  				int j = msecs_to_jiffies(500);
> > >  				schedule_timeout_uninterruptible(j);
> > >  			}
> > 
> > The compiler's whining about the mixed declarations and code, so I've
> > also done the following in my copy.
> > 
> > --b.
> > 
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index a3165a2..53c8ea9 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -587,9 +587,9 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
> >  		while (rqstp->rq_pages[i] == NULL) {
> >  			struct page *p = alloc_page(GFP_KERNEL);
> >  			if (!p) {
> > +				int j = msecs_to_jiffies(500);
> >  				if (kthread_should_stop())
> >  					return -EINTR;
> > -				int j = msecs_to_jiffies(500);
> >  				schedule_timeout_uninterruptible(j);
> >  			}
> >  			rqstp->rq_pages[i] = p;
> 
> Thanks Bruce. This reminds me though that I had a question about the
> above. I'm assuming that it's ok to return -EINTR without allocating
> all of the pages that we'll need to actually do the recv. This seems to
> be OK for all of the in-kernel callers of svc_recv...
> 
> Given that, is there any reason we need an uninterruptible sleep there?

I can't think of any reason either.

> It looks like when under heavy memory pressure, svc_recv could loop there
> for a long time. Allowing it to exit from that loop when signalled seems
> like it would be a good thing...

Yes, I think you're right.

--b.