Right now, nfsd keeps a lockd reference for each socket that it has
open. This is unnecessary and complicates the error handling on startup
and shutdown. Change it to just do a lockd_up when starting the first
nfsd thread just do a single lockd_down when taking down the last nfsd
thread. Because of the strange way the sv_count is handled, this
requires an extra flag to tell whether the nfsd_serv holds a reference
for lockd or not.
This patch also changes the error handling in nfsd_create_serv a bit
too. There doesn't seem to be any need to reset the nfssvc_boot time if
the nfsd startup failed.
Note though that this does change the user-visible behavior slightly.
Today, a lockd_up is done whenever a socket fd is handed off to the
kernel. With this change, lockd is brought up as soon as the first
thread is started. I think this makes more sense. If there are problems
in userspace, the old scheme had the possibility to start lockd long
before any nfsd threads were started. This patch helps minimize that
possibility.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfsctl.c | 10 ----------
fs/nfsd/nfssvc.c | 42 +++++++++++++++++++++++++++++-------------
2 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 9e8645a..b1c5be8 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -949,15 +949,8 @@ static ssize_t __write_ports_addfd(char *buf)
if (err != 0)
return err;
- err = lockd_up();
- if (err != 0) {
- svc_destroy(nfsd_serv);
- return err;
- }
-
err = svc_addsock(nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT);
if (err < 0) {
- lockd_down();
svc_destroy(nfsd_serv);
return err;
}
@@ -982,9 +975,6 @@ static ssize_t __write_ports_delfd(char *buf)
if (nfsd_serv != NULL)
len = svc_sock_names(nfsd_serv, buf,
SIMPLE_TRANSACTION_LIMIT, toclose);
- if (len >= 0)
- lockd_down();
-
kfree(toclose);
return len;
}
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index a06ea99..2e15db0 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -25,6 +25,7 @@
extern struct svc_program nfsd_program;
static int nfsd(void *vrqstp);
struct timeval nfssvc_boot;
+static bool nfsd_lockd_up;
/*
* nfsd_mutex protects nfsd_serv -- both the pointer itself and the members
@@ -183,9 +184,10 @@ int nfsd_nrthreads(void)
static void nfsd_last_thread(struct svc_serv *serv)
{
/* When last nfsd thread exits we need to do some clean-up */
- struct svc_xprt *xprt;
- list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list)
+ if (nfsd_lockd_up) {
lockd_down();
+ nfsd_lockd_up = false;
+ }
nfsd_serv = NULL;
nfsd_racache_shutdown();
nfs4_state_shutdown();
@@ -267,10 +269,9 @@ int nfsd_create_serv(void)
nfsd_serv = svc_create_pooled(&nfsd_program, nfsd_max_blksize,
nfsd_last_thread, nfsd, THIS_MODULE);
if (nfsd_serv == NULL)
- err = -ENOMEM;
- else
- set_max_drc();
+ return -ENOMEM;
+ set_max_drc();
do_gettimeofday(&nfssvc_boot); /* record boot time */
return err;
}
@@ -286,19 +287,11 @@ static int nfsd_init_socks(int port)
if (error < 0)
return error;
- error = lockd_up();
- if (error < 0)
- return error;
-
error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
SVC_SOCK_DEFAULTS);
if (error < 0)
return error;
- error = lockd_up();
- if (error < 0)
- return error;
-
return 0;
}
@@ -380,6 +373,7 @@ int
nfsd_svc(unsigned short port, int nrservs)
{
int error;
+ bool lockd_started = false;
mutex_lock(&nfsd_mutex);
dprintk("nfsd: creating service\n");
@@ -395,6 +389,16 @@ nfsd_svc(unsigned short port, int nrservs)
error = nfsd_racache_init(2*nrservs);
if (error<0)
goto out;
+
+ /* start lockd iff we're starting threads */
+ if ((nrservs > 0) && !nfsd_lockd_up) {
+ error = lockd_up();
+ if (error != 0)
+ goto out;
+ nfsd_lockd_up = true;
+ lockd_started = true;
+ }
+
error = nfs4_state_start();
if (error)
goto out;
@@ -416,12 +420,24 @@ nfsd_svc(unsigned short port, int nrservs)
* so subtract 1
*/
error = nfsd_serv->sv_nrthreads - 1;
+
+ /* if we brought all threads down, do a lockd_down */
+ if ((error == 0) && nfsd_lockd_up) {
+ lockd_down();
+ nfsd_lockd_up = false;
+ }
+
failure:
svc_destroy(nfsd_serv); /* Release server */
shutdown_state:
if (error < 0)
nfs4_state_shutdown();
out:
+ /* lockd_down if there was an error, and we did a lockd_up */
+ if (lockd_started && error < 0) {
+ lockd_down();
+ nfsd_lockd_up = false;
+ }
mutex_unlock(&nfsd_mutex);
return error;
}
--
1.5.5.6
-----Original Message-----
From: [email protected]
[mailto:[email protected]] On Behalf Of J. Bruce Fields
Sent: Wednesday, July 21, 2010 4:15 PM
To: Jeff Layton
Cc: [email protected]
Subject: Re: [PATCH 5/5] nfsd: just keep single lockd reference for nfsd
(try #4)
On Wed, Jul 21, 2010 at 09:21:41AM -0400, Jeff Layton wrote:
> Right now, nfsd keeps a lockd reference for each socket that it has
> open. This is unnecessary and complicates the error handling on
startup
> and shutdown. Change it to just do a lockd_up when starting the first
> nfsd thread just do a single lockd_down when taking down the last nfsd
> thread. Because of the strange way the sv_count is handled, this
> requires an extra flag to tell whether the nfsd_serv holds a reference
> for lockd or not.
>
> This patch also changes the error handling in nfsd_create_serv a bit
> too. There doesn't seem to be any need to reset the nfssvc_boot time
if
> the nfsd startup failed.
>
> Note though that this does change the user-visible behavior slightly.
> Today, a lockd_up is done whenever a socket fd is handed off to the
> kernel. With this change, lockd is brought up as soon as the first
> thread is started. I think this makes more sense. If there are
problems
> in userspace, the old scheme had the possibility to start lockd long
> before any nfsd threads were started. This patch helps minimize that
> possibility.
The nfs4 state startup has the same problem that I think lockd_up was
designed to solve.
There's a bunch of stuff that only makes sense to run when nrthreads
transitions from zero to nonzero, or vice-versa; so, if we stick them
all in one helper function I think it's cleaner and solves another minor
startup bug. Something like this?
(Incremental on top of your last patch, with some noise due to moving
stuff around to avoid forward references. I'll clean these up and
resend.)
Then we could also get rid of some of the individual flags (nfs4_init at
least), I think.
--b.
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 2e15db0..fd2524b 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -25,7 +25,6 @@
extern struct svc_program nfsd_program;
static int nfsd(void *vrqstp);
struct timeval nfssvc_boot;
-static bool nfsd_lockd_up;
/*
* nfsd_mutex protects nfsd_serv -- both the pointer itself and the
members
@@ -181,16 +180,79 @@ int nfsd_nrthreads(void)
return rv;
}
+static int nfsd_init_socks(int port)
+{
+ int error;
+ if (!list_empty(&nfsd_serv->sv_permsocks))
+ return 0;
+
+ error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
+ SVC_SOCK_DEFAULTS);
+ if (error < 0)
+ return error;
+
+ error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
+ SVC_SOCK_DEFAULTS);
+ if (error < 0)
+ return error;
Doesn't this leave something dangling if svc_create_xprt for "udp"
succeeds,
but svc_create_xprt for "tcp" fails?
ps
+
+ return 0;
+}
+
+static bool nfsd_up = false;
+
+static int nfsd_startup(unsigned short port, int nrservs)
+{
+ int ret;
+
+ /*
+ * Readahead param cache - will no-op if it already exists.
+ * (Note therefore results will be suboptimal if number of
+ * threads is modified after nfsd start.)
+ */
+ ret = nfsd_racache_init(2*nrservs);
+ if (ret)
+ return ret;
+ ret = nfsd_init_socks(port);
+ if (ret)
+ goto out_racache;
+ ret = lockd_up();
+ if (ret)
+ return ret;
+ ret = nfs4_state_start();
+ if (ret)
+ goto out_lockd;
+ nfsd_reset_versions();
+ nfsd_up = true;
+ return 0;
+out_lockd:
+ lockd_down();
+out_racache:
+ nfsd_racache_shutdown();
+ return ret;
+}
+
+static void nfsd_shutdown(void)
+{
+ /*
+ * write_ports can create the server without actually starting
+ * any threads--if we get shut down before any threads are
+ * started, the nfsd_last_thread will be run before any of this
+ * other initialization has been done.
+ */
+ if (!nfsd_up)
+ return;
+ nfs4_state_shutdown();
+ lockd_down();
+ nfsd_racache_shutdown();
+ nfsd_up = false;
+}
+
static void nfsd_last_thread(struct svc_serv *serv)
{
/* When last nfsd thread exits we need to do some clean-up */
- if (nfsd_lockd_up) {
- lockd_down();
- nfsd_lockd_up = false;
- }
nfsd_serv = NULL;
- nfsd_racache_shutdown();
- nfs4_state_shutdown();
+ nfsd_shutdown();
printk(KERN_WARNING "nfsd: last server has exited, flushing
export "
"cache\n");
@@ -276,25 +338,6 @@ int nfsd_create_serv(void)
return err;
}
-static int nfsd_init_socks(int port)
-{
- int error;
- if (!list_empty(&nfsd_serv->sv_permsocks))
- return 0;
-
- error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
- SVC_SOCK_DEFAULTS);
- if (error < 0)
- return error;
-
- error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
- SVC_SOCK_DEFAULTS);
- if (error < 0)
- return error;
-
- return 0;
-}
-
int nfsd_nrpools(void)
{
if (nfsd_serv == NULL)
@@ -369,11 +412,16 @@ int nfsd_set_nrthreads(int n, int *nthreads)
return err;
}
+/*
+ * Adjust the number of threads and return the new number of threads.
+ * This is also the function that starts the server if necessary, if
+ * this is the first time nrservs is nonzero.
+ */
int
nfsd_svc(unsigned short port, int nrservs)
{
int error;
- bool lockd_started = false;
+ bool first_thread;
mutex_lock(&nfsd_mutex);
dprintk("nfsd: creating service\n");
@@ -385,59 +433,33 @@ nfsd_svc(unsigned short port, int nrservs)
if (nrservs == 0 && nfsd_serv == NULL)
goto out;
- /* Readahead param cache - will no-op if it already exists */
- error = nfsd_racache_init(2*nrservs);
- if (error<0)
- goto out;
+ first_thread = (nfsd_serv->sv_nrthreads == 0) && (nrservs != 0);
- /* start lockd iff we're starting threads */
- if ((nrservs > 0) && !nfsd_lockd_up) {
- error = lockd_up();
- if (error != 0)
+ if (first_thread) {
+ error = nfsd_startup(port, nrservs);
+ if (error)
goto out;
- nfsd_lockd_up = true;
- lockd_started = true;
}
- error = nfs4_state_start();
- if (error)
- goto out;
-
- nfsd_reset_versions();
-
error = nfsd_create_serv();
if (error)
- goto shutdown_state;
-
- error = nfsd_init_socks(port);
- if (error)
- goto failure;
+ goto out_shutdown;
error = svc_set_num_threads(nfsd_serv, NULL, nrservs);
- if (error == 0)
- /* We are holding a reference to nfsd_serv which
- * we don't want to count in the return value,
- * so subtract 1
- */
- error = nfsd_serv->sv_nrthreads - 1;
-
- /* if we brought all threads down, do a lockd_down */
- if ((error == 0) && nfsd_lockd_up) {
- lockd_down();
- nfsd_lockd_up = false;
- }
+ if (error)
+ goto out_destroy;
+ /* We are holding a reference to nfsd_serv which
+ * we don't want to count in the return value,
+ * so subtract 1
+ */
+ error = nfsd_serv->sv_nrthreads - 1;
-failure:
+out_destroy:
svc_destroy(nfsd_serv); /* Release server */
-shutdown_state:
- if (error < 0)
- nfs4_state_shutdown();
+out_shutdown:
+ if (error < 0 && first_thread)
+ nfsd_shutdown();
out:
- /* lockd_down if there was an error, and we did a lockd_up */
- if (lockd_started && error < 0) {
- lockd_down();
- nfsd_lockd_up = false;
- }
mutex_unlock(&nfsd_mutex);
return error;
}
On Thu, 22 Jul 2010 07:59:39 -0400
<[email protected]> wrote:
>
>
> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of J. Bruce Fields
> Sent: Wednesday, July 21, 2010 4:15 PM
> To: Jeff Layton
> Cc: [email protected]
> Subject: Re: [PATCH 5/5] nfsd: just keep single lockd reference for nfsd
> (try #4)
>
> On Wed, Jul 21, 2010 at 09:21:41AM -0400, Jeff Layton wrote:
> > Right now, nfsd keeps a lockd reference for each socket that it has
> > open. This is unnecessary and complicates the error handling on
> startup
> > and shutdown. Change it to just do a lockd_up when starting the first
> > nfsd thread just do a single lockd_down when taking down the last nfsd
> > thread. Because of the strange way the sv_count is handled, this
> > requires an extra flag to tell whether the nfsd_serv holds a reference
> > for lockd or not.
> >
> > This patch also changes the error handling in nfsd_create_serv a bit
> > too. There doesn't seem to be any need to reset the nfssvc_boot time
> if
> > the nfsd startup failed.
> >
> > Note though that this does change the user-visible behavior slightly.
> > Today, a lockd_up is done whenever a socket fd is handed off to the
> > kernel. With this change, lockd is brought up as soon as the first
> > thread is started. I think this makes more sense. If there are
> problems
> > in userspace, the old scheme had the possibility to start lockd long
> > before any nfsd threads were started. This patch helps minimize that
> > possibility.
>
> The nfs4 state startup has the same problem that I think lockd_up was
> designed to solve.
>
> There's a bunch of stuff that only makes sense to run when nrthreads
> transitions from zero to nonzero, or vice-versa; so, if we stick them
> all in one helper function I think it's cleaner and solves another minor
> startup bug. Something like this?
>
> (Incremental on top of your last patch, with some noise due to moving
> stuff around to avoid forward references. I'll clean these up and
> resend.)
>
> Then we could also get rid of some of the individual flags (nfs4_init at
> least), I think.
>
> --b.
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 2e15db0..fd2524b 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -25,7 +25,6 @@
> extern struct svc_program nfsd_program;
> static int nfsd(void *vrqstp);
> struct timeval nfssvc_boot;
> -static bool nfsd_lockd_up;
>
> /*
> * nfsd_mutex protects nfsd_serv -- both the pointer itself and the
> members
> @@ -181,16 +180,79 @@ int nfsd_nrthreads(void)
> return rv;
> }
>
> +static int nfsd_init_socks(int port)
> +{
> + int error;
> + if (!list_empty(&nfsd_serv->sv_permsocks))
> + return 0;
> +
> + error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
> + SVC_SOCK_DEFAULTS);
> + if (error < 0)
> + return error;
> +
> + error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
> + SVC_SOCK_DEFAULTS);
> + if (error < 0)
> + return error;
>
> Doesn't this leave something dangling if svc_create_xprt for "udp"
> succeeds,
> but svc_create_xprt for "tcp" fails?
>
> ps
>
I think you're right. Note however that Bruce is just moving this
function around to avoid a forward declaration.
FWIW, I don't think this code is really used anymore -- at least
not with any reasonably modern rpc.nfsd program. Those all "hand off"
sockets explicitly to the kernel before bringing up threads, and the
sv_permsocks check short circuits that code.
So, this really amounts to a legacy interface and I'm not sure that we
want to go to any lengths to fix it.
--
Jeff Layton <[email protected]>
On Thu, Jul 22, 2010 at 07:59:39AM -0400, [email protected] wrote:
> +static int nfsd_init_socks(int port)
> +{
> + int error;
> + if (!list_empty(&nfsd_serv->sv_permsocks))
> + return 0;
> +
> + error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
> + SVC_SOCK_DEFAULTS);
> + if (error < 0)
> + return error;
> +
> + error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
> + SVC_SOCK_DEFAULTS);
> + if (error < 0)
> + return error;
>
> Doesn't this leave something dangling if svc_create_xprt for "udp"
> succeeds,
> but svc_create_xprt for "tcp" fails?
Yeah, but those sockets are all cleaned up when nfsd_serv is destroyed,
so I think we can live with this.
--b.
On Wed, Jul 21, 2010 at 09:21:41AM -0400, Jeff Layton wrote:
> Right now, nfsd keeps a lockd reference for each socket that it has
> open. This is unnecessary and complicates the error handling on startup
> and shutdown. Change it to just do a lockd_up when starting the first
> nfsd thread just do a single lockd_down when taking down the last nfsd
> thread. Because of the strange way the sv_count is handled, this
> requires an extra flag to tell whether the nfsd_serv holds a reference
> for lockd or not.
>
> This patch also changes the error handling in nfsd_create_serv a bit
> too. There doesn't seem to be any need to reset the nfssvc_boot time if
> the nfsd startup failed.
>
> Note though that this does change the user-visible behavior slightly.
> Today, a lockd_up is done whenever a socket fd is handed off to the
> kernel. With this change, lockd is brought up as soon as the first
> thread is started. I think this makes more sense. If there are problems
> in userspace, the old scheme had the possibility to start lockd long
> before any nfsd threads were started. This patch helps minimize that
> possibility.
The nfs4 state startup has the same problem that I think lockd_up was
designed to solve.
There's a bunch of stuff that only makes sense to run when nrthreads
transitions from zero to nonzero, or vice-versa; so, if we stick them
all in one helper function I think it's cleaner and solves another minor
startup bug. Something like this?
(Incremental on top of your last patch, with some noise due to moving
stuff around to avoid forward references. I'll clean these up and
resend.)
Then we could also get rid of some of the individual flags (nfs4_init at
least), I think.
--b.
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 2e15db0..fd2524b 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -25,7 +25,6 @@
extern struct svc_program nfsd_program;
static int nfsd(void *vrqstp);
struct timeval nfssvc_boot;
-static bool nfsd_lockd_up;
/*
* nfsd_mutex protects nfsd_serv -- both the pointer itself and the members
@@ -181,16 +180,79 @@ int nfsd_nrthreads(void)
return rv;
}
+static int nfsd_init_socks(int port)
+{
+ int error;
+ if (!list_empty(&nfsd_serv->sv_permsocks))
+ return 0;
+
+ error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
+ SVC_SOCK_DEFAULTS);
+ if (error < 0)
+ return error;
+
+ error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
+ SVC_SOCK_DEFAULTS);
+ if (error < 0)
+ return error;
+
+ return 0;
+}
+
+static bool nfsd_up = false;
+
+static int nfsd_startup(unsigned short port, int nrservs)
+{
+ int ret;
+
+ /*
+ * Readahead param cache - will no-op if it already exists.
+ * (Note therefore results will be suboptimal if number of
+ * threads is modified after nfsd start.)
+ */
+ ret = nfsd_racache_init(2*nrservs);
+ if (ret)
+ return ret;
+ ret = nfsd_init_socks(port);
+ if (ret)
+ goto out_racache;
+ ret = lockd_up();
+ if (ret)
+ return ret;
+ ret = nfs4_state_start();
+ if (ret)
+ goto out_lockd;
+ nfsd_reset_versions();
+ nfsd_up = true;
+ return 0;
+out_lockd:
+ lockd_down();
+out_racache:
+ nfsd_racache_shutdown();
+ return ret;
+}
+
+static void nfsd_shutdown(void)
+{
+ /*
+ * write_ports can create the server without actually starting
+ * any threads--if we get shut down before any threads are
+ * started, the nfsd_last_thread will be run before any of this
+ * other initialization has been done.
+ */
+ if (!nfsd_up)
+ return;
+ nfs4_state_shutdown();
+ lockd_down();
+ nfsd_racache_shutdown();
+ nfsd_up = false;
+}
+
static void nfsd_last_thread(struct svc_serv *serv)
{
/* When last nfsd thread exits we need to do some clean-up */
- if (nfsd_lockd_up) {
- lockd_down();
- nfsd_lockd_up = false;
- }
nfsd_serv = NULL;
- nfsd_racache_shutdown();
- nfs4_state_shutdown();
+ nfsd_shutdown();
printk(KERN_WARNING "nfsd: last server has exited, flushing export "
"cache\n");
@@ -276,25 +338,6 @@ int nfsd_create_serv(void)
return err;
}
-static int nfsd_init_socks(int port)
-{
- int error;
- if (!list_empty(&nfsd_serv->sv_permsocks))
- return 0;
-
- error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
- SVC_SOCK_DEFAULTS);
- if (error < 0)
- return error;
-
- error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
- SVC_SOCK_DEFAULTS);
- if (error < 0)
- return error;
-
- return 0;
-}
-
int nfsd_nrpools(void)
{
if (nfsd_serv == NULL)
@@ -369,11 +412,16 @@ int nfsd_set_nrthreads(int n, int *nthreads)
return err;
}
+/*
+ * Adjust the number of threads and return the new number of threads.
+ * This is also the function that starts the server if necessary, if
+ * this is the first time nrservs is nonzero.
+ */
int
nfsd_svc(unsigned short port, int nrservs)
{
int error;
- bool lockd_started = false;
+ bool first_thread;
mutex_lock(&nfsd_mutex);
dprintk("nfsd: creating service\n");
@@ -385,59 +433,33 @@ nfsd_svc(unsigned short port, int nrservs)
if (nrservs == 0 && nfsd_serv == NULL)
goto out;
- /* Readahead param cache - will no-op if it already exists */
- error = nfsd_racache_init(2*nrservs);
- if (error<0)
- goto out;
+ first_thread = (nfsd_serv->sv_nrthreads == 0) && (nrservs != 0);
- /* start lockd iff we're starting threads */
- if ((nrservs > 0) && !nfsd_lockd_up) {
- error = lockd_up();
- if (error != 0)
+ if (first_thread) {
+ error = nfsd_startup(port, nrservs);
+ if (error)
goto out;
- nfsd_lockd_up = true;
- lockd_started = true;
}
- error = nfs4_state_start();
- if (error)
- goto out;
-
- nfsd_reset_versions();
-
error = nfsd_create_serv();
if (error)
- goto shutdown_state;
-
- error = nfsd_init_socks(port);
- if (error)
- goto failure;
+ goto out_shutdown;
error = svc_set_num_threads(nfsd_serv, NULL, nrservs);
- if (error == 0)
- /* We are holding a reference to nfsd_serv which
- * we don't want to count in the return value,
- * so subtract 1
- */
- error = nfsd_serv->sv_nrthreads - 1;
-
- /* if we brought all threads down, do a lockd_down */
- if ((error == 0) && nfsd_lockd_up) {
- lockd_down();
- nfsd_lockd_up = false;
- }
+ if (error)
+ goto out_destroy;
+ /* We are holding a reference to nfsd_serv which
+ * we don't want to count in the return value,
+ * so subtract 1
+ */
+ error = nfsd_serv->sv_nrthreads - 1;
-failure:
+out_destroy:
svc_destroy(nfsd_serv); /* Release server */
-shutdown_state:
- if (error < 0)
- nfs4_state_shutdown();
+out_shutdown:
+ if (error < 0 && first_thread)
+ nfsd_shutdown();
out:
- /* lockd_down if there was an error, and we did a lockd_up */
- if (lockd_started && error < 0) {
- lockd_down();
- nfsd_lockd_up = false;
- }
mutex_unlock(&nfsd_mutex);
return error;
}
On Wed, Jul 21, 2010 at 04:14:37PM -0400, J. Bruce Fields wrote:
> (Incremental on top of your last patch, with some noise due to moving
> stuff around to avoid forward references. I'll clean these up and
> resend.)
>
> Then we could also get rid of some of the individual flags (nfs4_init at
> least), I think.
Here's what I'm thinking of applying. (Lightly tested.)
--b.
From: Jeff Layton <[email protected]>
If someone tries to shut down the laundry_wq while it isn't up it'll
cause an oops.
This can happen because write_ports can create a nfsd_svc before we
really start the nfs server, and we may fail before the server is ever
started.
Also make sure state is shutdown on error paths in nfsd_svc().
Use a common global nfsd_up flag instead of nfs4_init, and create common
helper functions for nfsd start/shutdown, as there will be other work
that we want done only when we the number of nfsd threads transitions
between zero and nonzero.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 12 +-----------
fs/nfsd/nfssvc.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
2 files changed, 43 insertions(+), 20 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 182448f..9cc3b78 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -51,7 +51,6 @@ static time_t boot_time;
static u32 current_ownerid = 1;
static u32 current_fileid = 1;
static u32 current_delegid = 1;
-static u32 nfs4_init;
static stateid_t zerostateid; /* bits all 0 */
static stateid_t onestateid; /* bits all 1 */
static u64 current_sessionid = 1;
@@ -4071,16 +4070,8 @@ out_free_laundry:
int
nfs4_state_start(void)
{
- int ret;
-
- if (nfs4_init)
- return 0;
nfsd4_load_reboot_recovery_data();
- ret = __nfs4_state_start();
- if (ret)
- return ret;
- nfs4_init = 1;
- return 0;
+ return __nfs4_state_start();
}
static void
@@ -4115,7 +4106,6 @@ __nfs4_state_shutdown(void)
}
nfsd4_shutdown_recdir();
- nfs4_init = 0;
}
void
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 06b2a26..d7a4d7b 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -180,6 +180,31 @@ int nfsd_nrthreads(void)
return rv;
}
+static bool nfsd_up = false;
+
+static int nfsd_startup(unsigned short port, int nrservs)
+{
+ int ret;
+
+ ret = nfs4_state_start();
+ nfsd_up = true;
+ return ret;
+}
+
+static void nfsd_shutdown(void)
+{
+ /*
+ * write_ports can create the server without actually starting
+ * any threads--if we get shut down before any threads are
+ * started, then nfsd_last_thread will be run before any of this
+ * other initialization has been done.
+ */
+ if (!nfsd_up)
+ return;
+ nfs4_state_shutdown();
+ nfsd_up = false;
+}
+
static void nfsd_last_thread(struct svc_serv *serv)
{
/* When last nfsd thread exits we need to do some clean-up */
@@ -188,7 +213,7 @@ static void nfsd_last_thread(struct svc_serv *serv)
lockd_down();
nfsd_serv = NULL;
nfsd_racache_shutdown();
- nfs4_state_shutdown();
+ nfsd_shutdown();
printk(KERN_WARNING "nfsd: last server has exited, flushing export "
"cache\n");
@@ -380,6 +405,7 @@ int
nfsd_svc(unsigned short port, int nrservs)
{
int error;
+ bool first_thread;
mutex_lock(&nfsd_mutex);
dprintk("nfsd: creating service\n");
@@ -395,19 +421,23 @@ nfsd_svc(unsigned short port, int nrservs)
error = nfsd_racache_init(2*nrservs);
if (error<0)
goto out;
- error = nfs4_state_start();
- if (error)
- goto out;
+
+ first_thread = (nfsd_serv->sv_nrthreads == 0) && (nrservs != 0);
+
+ if (first_thread) {
+ error = nfsd_startup(port, nrservs);
+ if (error)
+ goto out;
+ }
nfsd_reset_versions();
error = nfsd_create_serv();
-
if (error)
- goto out;
+ goto out_shutdown;
error = nfsd_init_socks(port);
if (error)
- goto failure;
+ goto out_destroy;
error = svc_set_num_threads(nfsd_serv, NULL, nrservs);
if (error == 0)
@@ -416,9 +446,12 @@ nfsd_svc(unsigned short port, int nrservs)
* so subtract 1
*/
error = nfsd_serv->sv_nrthreads - 1;
- failure:
+out_destroy:
svc_destroy(nfsd_serv); /* Release server */
- out:
+out_shutdown:
+ if (error < 0 && first_thread)
+ nfsd_shutdown();
+out:
mutex_unlock(&nfsd_mutex);
return error;
}
--
1.7.0.4
From: Jeff Layton <[email protected]>
The refcounting for nfsd is a little goofy. What happens is that we
create the nfsd RPC service, attach sockets to it but don't actually
start the threads until someone writes to the "threads" procfile. To do
this, __write_ports_addfd will create the nfsd service and then will
decrement the refcount when exiting but won't actually destroy the
service.
This is fine when there aren't errors, but when there are this can
cause later attempts to start nfsd to fail. nfsd_serv will be set,
and that causes __write_versions to return EBUSY.
Fix this by calling svc_destroy on nfsd_serv when this function is
going to return error.
Signed-off-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfsctl.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 508941c..af7469e 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -950,14 +950,18 @@ static ssize_t __write_ports_addfd(char *buf)
return err;
err = lockd_up();
- if (err != 0)
- goto out;
+ if (err != 0) {
+ svc_destroy(nfsd_serv);
+ return err;
+ }
err = svc_addsock(nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT);
- if (err < 0)
+ if (err < 0) {
lockd_down();
+ svc_destroy(nfsd_serv);
+ return err;
+ }
-out:
/* Decrease the count, but don't shut down the service */
nfsd_serv->sv_nrthreads--;
return err;
--
1.7.0.4
From: Jeff Layton <[email protected]>
__write_ports_addxprt calls nfsd_create_serv. That increases the
refcount of nfsd_serv (which is tracked in sv_nrthreads). The service
only decrements the thread count on error, not on success like
__write_ports_addfd does, so using this interface leaves the nfsd
thread count high.
Fix this by having this function call svc_destroy() on error to release
the reference (and possibly to tear down the service) and simply
decrement the refcount without tearing down the service on success.
This makes the sv_threads handling work basically the same in both
__write_ports_addxprt and __write_ports_addfd.
Signed-off-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfsctl.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index af7469e..9e8645a 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1018,6 +1018,9 @@ static ssize_t __write_ports_addxprt(char *buf)
PF_INET6, port, SVC_SOCK_ANONYMOUS);
if (err < 0 && err != -EAFNOSUPPORT)
goto out_close;
+
+ /* Decrease the count, but don't shut down the service */
+ nfsd_serv->sv_nrthreads--;
return 0;
out_close:
xprt = svc_find_xprt(nfsd_serv, transport, PF_INET, port);
@@ -1026,8 +1029,7 @@ out_close:
svc_xprt_put(xprt);
}
out_err:
- /* Decrease the count, but don't shut down the service */
- nfsd_serv->sv_nrthreads--;
+ svc_destroy(nfsd_serv);
return err;
}
--
1.7.0.4
From: Jeff Layton <[email protected]>
There doesn't seem to be any need to reset the nfssvc_boot time if the
nfsd startup failed.
Signed-off-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfssvc.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index d7a4d7b..a631ea6 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -292,10 +292,9 @@ int nfsd_create_serv(void)
nfsd_serv = svc_create_pooled(&nfsd_program, nfsd_max_blksize,
nfsd_last_thread, nfsd, THIS_MODULE);
if (nfsd_serv == NULL)
- err = -ENOMEM;
- else
- set_max_drc();
+ return -ENOMEM;
+ set_max_drc();
do_gettimeofday(&nfssvc_boot); /* record boot time */
return err;
}
--
1.7.0.4
From: Jeff Layton <[email protected]>
Right now, nfsd keeps a lockd reference for each socket that it has
open. This is unnecessary and complicates the error handling on
startup and shutdown. Change it to just do a lockd_up when starting
the first nfsd thread just do a single lockd_down when taking down the
last nfsd thread. Because of the strange way the sv_count is handled
this requires an extra flag to tell whether the nfsd_serv holds a
reference for lockd or not.
Signed-off-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfsctl.c | 10 ----------
fs/nfsd/nfssvc.c | 25 ++++++++++++++-----------
2 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 9e8645a..b1c5be8 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -949,15 +949,8 @@ static ssize_t __write_ports_addfd(char *buf)
if (err != 0)
return err;
- err = lockd_up();
- if (err != 0) {
- svc_destroy(nfsd_serv);
- return err;
- }
-
err = svc_addsock(nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT);
if (err < 0) {
- lockd_down();
svc_destroy(nfsd_serv);
return err;
}
@@ -982,9 +975,6 @@ static ssize_t __write_ports_delfd(char *buf)
if (nfsd_serv != NULL)
len = svc_sock_names(nfsd_serv, buf,
SIMPLE_TRANSACTION_LIMIT, toclose);
- if (len >= 0)
- lockd_down();
-
kfree(toclose);
return len;
}
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index a631ea6..8a556ff 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -186,8 +186,16 @@ static int nfsd_startup(unsigned short port, int nrservs)
{
int ret;
+ ret = lockd_up();
+ if (ret)
+ return ret;
ret = nfs4_state_start();
+ if (ret)
+ goto out_lockd;
nfsd_up = true;
+ return 0;
+out_lockd:
+ lockd_down();
return ret;
}
@@ -201,6 +209,7 @@ static void nfsd_shutdown(void)
*/
if (!nfsd_up)
return;
+ lockd_down();
nfs4_state_shutdown();
nfsd_up = false;
}
@@ -208,9 +217,6 @@ static void nfsd_shutdown(void)
static void nfsd_last_thread(struct svc_serv *serv)
{
/* When last nfsd thread exits we need to do some clean-up */
- struct svc_xprt *xprt;
- list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list)
- lockd_down();
nfsd_serv = NULL;
nfsd_racache_shutdown();
nfsd_shutdown();
@@ -310,19 +316,11 @@ static int nfsd_init_socks(int port)
if (error < 0)
return error;
- error = lockd_up();
- if (error < 0)
- return error;
-
error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
SVC_SOCK_DEFAULTS);
if (error < 0)
return error;
- error = lockd_up();
- if (error < 0)
- return error;
-
return 0;
}
@@ -400,6 +398,11 @@ int nfsd_set_nrthreads(int n, int *nthreads)
return err;
}
+/*
+ * Adjust the number of threads and return the new number of threads.
+ * This is also the function that starts the server if necessary, if
+ * this is the first time nrservs is nonzero.
+ */
int
nfsd_svc(unsigned short port, int nrservs)
{
--
1.7.0.4
This is just cleanup--it's harmless to call nfsd_rachache_init,
nfsd_init_socks, and nfsd_reset_versions more than once. But there's no
point to it.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfssvc.c | 69 +++++++++++++++++++++++++++--------------------------
1 files changed, 35 insertions(+), 34 deletions(-)
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 8a556ff..62a6c44 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -180,22 +180,54 @@ int nfsd_nrthreads(void)
return rv;
}
+static int nfsd_init_socks(int port)
+{
+ int error;
+ if (!list_empty(&nfsd_serv->sv_permsocks))
+ return 0;
+
+ error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
+ SVC_SOCK_DEFAULTS);
+ if (error < 0)
+ return error;
+
+ error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
+ SVC_SOCK_DEFAULTS);
+ if (error < 0)
+ return error;
+
+ return 0;
+}
+
static bool nfsd_up = false;
static int nfsd_startup(unsigned short port, int nrservs)
{
int ret;
-
+ /*
+ * Readahead param cache - will no-op if it already exists.
+ * (Note therefore results will be suboptimal if number of
+ * threads is modified after nfsd start.)
+ */
+ ret = nfsd_racache_init(2*nrservs);
+ if (ret)
+ return ret;
+ ret = nfsd_init_socks(port);
+ if (ret)
+ goto out_racache;
ret = lockd_up();
if (ret)
return ret;
ret = nfs4_state_start();
if (ret)
goto out_lockd;
+ nfsd_reset_versions();
nfsd_up = true;
return 0;
out_lockd:
lockd_down();
+out_racache:
+ nfsd_racache_shutdown();
return ret;
}
@@ -209,8 +241,9 @@ static void nfsd_shutdown(void)
*/
if (!nfsd_up)
return;
- lockd_down();
nfs4_state_shutdown();
+ lockd_down();
+ nfsd_racache_shutdown();
nfsd_up = false;
}
@@ -218,7 +251,6 @@ static void nfsd_last_thread(struct svc_serv *serv)
{
/* When last nfsd thread exits we need to do some clean-up */
nfsd_serv = NULL;
- nfsd_racache_shutdown();
nfsd_shutdown();
printk(KERN_WARNING "nfsd: last server has exited, flushing export "
@@ -305,25 +337,6 @@ int nfsd_create_serv(void)
return err;
}
-static int nfsd_init_socks(int port)
-{
- int error;
- if (!list_empty(&nfsd_serv->sv_permsocks))
- return 0;
-
- error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
- SVC_SOCK_DEFAULTS);
- if (error < 0)
- return error;
-
- error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
- SVC_SOCK_DEFAULTS);
- if (error < 0)
- return error;
-
- return 0;
-}
-
int nfsd_nrpools(void)
{
if (nfsd_serv == NULL)
@@ -419,11 +432,6 @@ nfsd_svc(unsigned short port, int nrservs)
if (nrservs == 0 && nfsd_serv == NULL)
goto out;
- /* Readahead param cache - will no-op if it already exists */
- error = nfsd_racache_init(2*nrservs);
- if (error<0)
- goto out;
-
first_thread = (nfsd_serv->sv_nrthreads == 0) && (nrservs != 0);
if (first_thread) {
@@ -431,16 +439,9 @@ nfsd_svc(unsigned short port, int nrservs)
if (error)
goto out;
}
-
- nfsd_reset_versions();
-
error = nfsd_create_serv();
if (error)
goto out_shutdown;
- error = nfsd_init_socks(port);
- if (error)
- goto out_destroy;
-
error = svc_set_num_threads(nfsd_serv, NULL, nrservs);
if (error == 0)
/* We are holding a reference to nfsd_serv which
--
1.7.0.4
More idiomatic to put the error case in the if clause.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfssvc.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 62a6c44..92173bd 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -443,12 +443,13 @@ nfsd_svc(unsigned short port, int nrservs)
if (error)
goto out_shutdown;
error = svc_set_num_threads(nfsd_serv, NULL, nrservs);
- if (error == 0)
- /* We are holding a reference to nfsd_serv which
- * we don't want to count in the return value,
- * so subtract 1
- */
- error = nfsd_serv->sv_nrthreads - 1;
+ if (error)
+ goto out_destroy;
+ /* We are holding a reference to nfsd_serv which
+ * we don't want to count in the return value,
+ * so subtract 1
+ */
+ error = nfsd_serv->sv_nrthreads - 1;
out_destroy:
svc_destroy(nfsd_serv); /* Release server */
out_shutdown:
--
1.7.0.4
On Wed, 21 Jul 2010 19:24:32 -0400
"J. Bruce Fields" <[email protected]> wrote:
> On Wed, Jul 21, 2010 at 04:14:37PM -0400, J. Bruce Fields wrote:
> > (Incremental on top of your last patch, with some noise due to moving
> > stuff around to avoid forward references. I'll clean these up and
> > resend.)
> >
> > Then we could also get rid of some of the individual flags (nfs4_init at
> > least), I think.
>
> Here's what I'm thinking of applying. (Lightly tested.)
>
I gave them some basic smoke testing and looked them over and they seem
to be ok. Unfortunately, I don't really have an exhaustive test for
this sort of thing, but it does seem to fix the problem that
was originally reported to me.
--
Jeff Layton <[email protected]>
On Thu, Jul 22, 2010 at 01:40:26PM -0400, Jeff Layton wrote:
> On Wed, 21 Jul 2010 19:24:32 -0400
> "J. Bruce Fields" <[email protected]> wrote:
>
> > On Wed, Jul 21, 2010 at 04:14:37PM -0400, J. Bruce Fields wrote:
> > > (Incremental on top of your last patch, with some noise due to moving
> > > stuff around to avoid forward references. I'll clean these up and
> > > resend.)
> > >
> > > Then we could also get rid of some of the individual flags (nfs4_init at
> > > least), I think.
> >
> > Here's what I'm thinking of applying. (Lightly tested.)
> >
>
> I gave them some basic smoke testing and looked them over and they seem
> to be ok. Unfortunately, I don't really have an exhaustive test for
> this sort of thing, but it does seem to fix the problem that
> was originally reported to me.
I also experimented a little with bringing nfs and rpcbind up and down
and didn't see any problems.
Applied for 2.6.36.
--b.