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 <[email protected]>
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 <[email protected]>
Reviewed-by: NeilBrown <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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(¤t->sighand->siglock);
- recalc_sigpending();
- spin_unlock_irq(¤t->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
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;
On Sun, 10 Feb 2008 19:47:59 -0500
"J. Bruce Fields" <[email protected]> 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 <[email protected]>
Oops, did I forget to send this? I don't' see it in the archives.
Apologies:
On Mon, Feb 11, 2008 at 12:22:08AM +0000, bfields wrote:
> On Thu, Feb 07, 2008 at 04:34:55PM -0500, Jeff Layton wrote:
> ...
> > 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
> ...
> > @@ -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;
>
> The following svc_recv call will check kthread_should_stop() pretty
> early on, so I don't believe this is necessary?
>
> Assuming that's correct, I've deleted these lines and applied the rest.
>
> --b.
On Mon, 11 Feb 2008 10:29:38 -0500
"J. Bruce Fields" <[email protected]> wrote:
> Oops, did I forget to send this? I don't' see it in the archives.
> Apologies:
>
> On Mon, Feb 11, 2008 at 12:22:08AM +0000, bfields wrote:
> > On Thu, Feb 07, 2008 at 04:34:55PM -0500, Jeff Layton wrote:
> > ...
> > > 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
> > ...
> > > @@ -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;
> >
> > The following svc_recv call will check kthread_should_stop() pretty
> > early on, so I don't believe this is necessary?
> >
> > Assuming that's correct, I've deleted these lines and applied the rest.
> >
Sounds reasonable...ACK
--
Jeff Layton <[email protected]>
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" <[email protected]> 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.