[PATCH 0/5] nfsd: fix error handling in write_ports interfaces (resend)

[PATCH 0/5] nfsd: fix error handling in write_ports interfaces (resend)

[Jeff Layton]

Hi Bruce,

This patchset is a resend of the set I sent a month or so ago, with
a couple of other patches to fix related problems that came to light
as we were discussing them.

It's changed a bit from the original set by virtue of the fact that
patch #5 simplifies the lockd_up/down handling. Let me know if you
see any problems with the set and I'll work on addressing them.

Thanks,

Jeff Layton (5):
  nfsd: don't try to shut down nfs4 state handling unless it's up
  nfsd: fix error handling when starting nfsd with rpcbind down
  nfsd: fix error handling in __write_ports_addxprt
  nfsd: shut down NFSv4 state when nfsd_svc encounters an error
  nfsd: just keep single lockd reference for nfsd

 fs/nfsd/nfs4state.c |    2 ++
 fs/nfsd/nfsctl.c    |   20 ++++++++------------
 fs/nfsd/nfssvc.c    |   36 +++++++++++++++++-------------------
 3 files changed, 27 insertions(+), 31 deletions(-)

[PATCH 1/5] nfsd: don't try to shut down nfs4 state handling unless it's up

[Jeff Layton]

If someone tries to shut down the laundry_wq while it isn't up it'll
cause an oops.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4state.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4a27347..0b396f8 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4116,6 +4116,8 @@ __nfs4_state_shutdown(void)
 void
 nfs4_state_shutdown(void)
 {
+	if (!nfs4_init)
+		return;
 	cancel_rearming_delayed_workqueue(laundry_wq, &laundromat_work);
 	destroy_workqueue(laundry_wq);
 	locks_end_grace(&nfsd4_manager);
-- 
1.5.5.6

Re: [PATCH 1/5] nfsd: don't try to shut down nfs4 state handling unless it's up

[J. Bruce Fields]

On Mon, Jul 19, 2010 at 04:50:04PM -0400, Jeff Layton wrote:
> If someone tries to shut down the laundry_wq while it isn't up it'll
> cause an oops.

nfs4_state_shutdown is called only from nfsd_last_thread, which is
called only after svc_create_pooled, which is called only when
nfs4_state_start() succeeds.

If you've actually seen this happen then there's something going on that
I don't understand.

--b.

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfsd/nfs4state.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4a27347..0b396f8 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4116,6 +4116,8 @@ __nfs4_state_shutdown(void)
>  void
>  nfs4_state_shutdown(void)
>  {
> +	if (!nfs4_init)
> +		return;
>  	cancel_rearming_delayed_workqueue(laundry_wq, &laundromat_work);
>  	destroy_workqueue(laundry_wq);
>  	locks_end_grace(&nfsd4_manager);
> -- 
> 1.5.5.6
> 

Re: [PATCH 1/5] nfsd: don't try to shut down nfs4 state handling unless it's up

[Jeff Layton]

On Mon, 19 Jul 2010 18:08:37 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Jul 19, 2010 at 04:50:04PM -0400, Jeff Layton wrote:
> > If someone tries to shut down the laundry_wq while it isn't up it'll
> > cause an oops.
> 
> nfs4_state_shutdown is called only from nfsd_last_thread, which is
> called only after svc_create_pooled, which is called only when
> nfs4_state_start() succeeds.
> 
> If you've actually seen this happen then there's something going on that
> I don't understand.
> 
> --b.

I've not seen this in current code, but this delta is a prerequisite
for the next patch in the series. It's possible for nfsd_serv to be
created (via the write_ports interface), but for the threads file never
to be written.

With the next patch in the series, the nfsd_serv can be torn down in an
error condition before the threads file is ever written and at that
point this patch is required to prevent panics.

Cheers,
-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 2/5] nfsd: fix error handling when starting nfsd with rpcbind down

[Jeff Layton]

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 <jlayton@redhat.com>
---
 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.5.5.6

[PATCH 3/5] nfsd: fix error handling in __write_ports_addxprt

[Jeff Layton]

__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 <jlayton@redhat.com>
---
 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.5.5.6

[PATCH 4/5] nfsd: shut down NFSv4 state when nfsd_svc encounters an error

[Jeff Layton]

Currently, it's left up in some situations. That could cause the grace
period to be shorter than it should be (along with other problems).

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfssvc.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 06b2a26..a06ea99 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -402,9 +402,9 @@ nfsd_svc(unsigned short port, int nrservs)
 	nfsd_reset_versions();
 
 	error = nfsd_create_serv();
-
 	if (error)
-		goto out;
+		goto shutdown_state;
+
 	error = nfsd_init_socks(port);
 	if (error)
 		goto failure;
@@ -416,9 +416,12 @@ nfsd_svc(unsigned short port, int nrservs)
 		 * so subtract 1
 		 */
 		error = nfsd_serv->sv_nrthreads - 1;
- failure:
+failure:
 	svc_destroy(nfsd_serv);		/* Release server */
- out:
+shutdown_state:
+	if (error < 0)
+		nfs4_state_shutdown();
+out:
 	mutex_unlock(&nfsd_mutex);
 	return error;
 }
-- 
1.5.5.6

[PATCH 5/5] nfsd: just keep single lockd reference for nfsd

[Jeff Layton]

This patch should replace the other patches that I proposed to make
sure that each sv_permsock entry holds a lockd refrence.

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 creating
the nfsd_serv and just do a single lockd_down when taking down the
last nfsd thread.

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 when someone writes a text socktype and port to the portlist file
prior to starting nfsd, lockd is not started when nfsd threads are
brought up. With this change, it will be started any time that
the nfsd_serv is created. I'm making the assumption that that's not a
problem. If it is then we'll need to take a different approach to fixing
this.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfsctl.c |   10 ----------
 fs/nfsd/nfssvc.c |   25 ++++++++++---------------
 2 files changed, 10 insertions(+), 25 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..6b59d32 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -183,9 +183,7 @@ 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)
-		lockd_down();
+	lockd_down();
 	nfsd_serv = NULL;
 	nfsd_racache_shutdown();
 	nfs4_state_shutdown();
@@ -264,13 +262,18 @@ int nfsd_create_serv(void)
 			nfsd_max_blksize /= 2;
 	}
 
+	err = lockd_up();
+	if (err != 0)
+		return err;
+
 	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();
+	if (nfsd_serv == NULL) {
+		lockd_down();
+		return -ENOMEM;
+	}
 
+	set_max_drc();
 	do_gettimeofday(&nfssvc_boot);		/* record boot time */
 	return err;
 }
@@ -286,19 +289,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;
 }
 
-- 
1.5.5.6

Re: [PATCH 5/5] nfsd: just keep single lockd reference for nfsd

[J. Bruce Fields]

On Mon, Jul 19, 2010 at 04:50:08PM -0400, Jeff Layton wrote:
> This patch should replace the other patches that I proposed to make
> sure that each sv_permsock entry holds a lockd refrence.
> 
> 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 creating
> the nfsd_serv and just do a single lockd_down when taking down the
> last nfsd thread.
> 
> 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 when someone writes a text socktype and port to the portlist file
> prior to starting nfsd, lockd is not started when nfsd threads are
> brought up. With this change, it will be started any time that
> the nfsd_serv is created.

OK, so it may be started earlier in the process than it was before.

> I'm making the assumption that that's not a
> problem. If it is then we'll need to take a different approach to fixing
> this.

Especially in the case of a later failure, it does worry me a little
that we could leave lockd running without having started nfsd.

Is there a clean way to defer starting lock till we start the first
server threads?  Maybe fs/nfsd/nfssvc.c:nfsd() could (at the start,
while under nfsd_mutex), check if lockd is up and start it if not??  But
I think that can leave us calling lockd_down and not lockd_up in some
cases.  Maybe start lockd in svc_set_num_threads()?

--b.

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfsd/nfsctl.c |   10 ----------
>  fs/nfsd/nfssvc.c |   25 ++++++++++---------------
>  2 files changed, 10 insertions(+), 25 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..6b59d32 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -183,9 +183,7 @@ 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)
> -		lockd_down();
> +	lockd_down();
>  	nfsd_serv = NULL;
>  	nfsd_racache_shutdown();
>  	nfs4_state_shutdown();
> @@ -264,13 +262,18 @@ int nfsd_create_serv(void)
>  			nfsd_max_blksize /= 2;
>  	}
>  
> +	err = lockd_up();
> +	if (err != 0)
> +		return err;
> +
>  	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();
> +	if (nfsd_serv == NULL) {
> +		lockd_down();
> +		return -ENOMEM;
> +	}
>  
> +	set_max_drc();
>  	do_gettimeofday(&nfssvc_boot);		/* record boot time */
>  	return err;
>  }
> @@ -286,19 +289,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;
>  }
>  
> -- 
> 1.5.5.6
> 

Re: [PATCH 5/5] nfsd: just keep single lockd reference for nfsd

[Jeff Layton]

On Tue, 20 Jul 2010 11:02:37 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Jul 19, 2010 at 04:50:08PM -0400, Jeff Layton wrote:
> > This patch should replace the other patches that I proposed to make
> > sure that each sv_permsock entry holds a lockd refrence.
> > 
> > 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 creating
> > the nfsd_serv and just do a single lockd_down when taking down the
> > last nfsd thread.
> > 
> > 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 when someone writes a text socktype and port to the portlist file
> > prior to starting nfsd, lockd is not started when nfsd threads are
> > brought up. With this change, it will be started any time that
> > the nfsd_serv is created.
> 
> OK, so it may be started earlier in the process than it was before.
> 
> > I'm making the assumption that that's not a
> > problem. If it is then we'll need to take a different approach to fixing
> > this.
> 
> Especially in the case of a later failure, it does worry me a little
> that we could leave lockd running without having started nfsd.
> 
> Is there a clean way to defer starting lock till we start the first
> server threads?  Maybe fs/nfsd/nfssvc.c:nfsd() could (at the start,
> while under nfsd_mutex), check if lockd is up and start it if not??  But
> I think that can leave us calling lockd_down and not lockd_up in some
> cases.  Maybe start lockd in svc_set_num_threads()?
> 

That's a good point. We do have to consider that starting lockd starts
the grace period...

I suppose we could add a global flag that indicates whether the
nfsd_serv currently holds a lockd reference yet, and only conditionally
do the lockd_down if it does. That seems pretty kludgey, but I don't
really see a better way to handle it without a more substantial
overhaul.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/5] nfsd: fix error handling in write_ports interfaces (resend)

[J. Bruce Fields]

On Mon, Jul 19, 2010 at 04:50:03PM -0400, Jeff Layton wrote:
> This patchset is a resend of the set I sent a month or so ago, with
> a couple of other patches to fix related problems that came to light
> as we were discussing them.

So, rpc.nfsd starts the nfs server by more or less:

	1. write nfs versions to /proc/fs/nfsd/versions
	2. write tcp socket fd to /proc/fs/nfsd/portlist
	3. write udp socket fd to /proc/fs/nfsd/portlist
	4. write number of threads to /proc/fs/nfsd/threads

A failure anywhere between step 2 and 4 leaves the server in an odd
state: it's created, but not really running yet.

Your patches help when it's step 2 that fails.  Probably that's the most
common case, so good.

But what if we step 3 fails?  Or rpc.nfsd encounters some other error
between steps 2 and 4?  Does it have any way to clean up?  (Is starting
a single thread and then stopping it the only way to destroy the server
at that point?)  And what if rpc.nfsd is interrupted before it gets to
step 4?

The whole interface seems strange and fragile.

The correct thing may be to apply your patches anyway, as long as they
solve a problem and don't make things any worse.

But I fear we'll be spending more time on corner cases in the future.

As a first exercise, maybe we should figure out how the interface would
look if we had the opportunity to start over from scratch.

--b.

> It's changed a bit from the original set by virtue of the fact that
> patch #5 simplifies the lockd_up/down handling. Let me know if you
> see any problems with the set and I'll work on addressing them.
> 
> Thanks,
> 
> Jeff Layton (5):
>   nfsd: don't try to shut down nfs4 state handling unless it's up
>   nfsd: fix error handling when starting nfsd with rpcbind down
>   nfsd: fix error handling in __write_ports_addxprt
>   nfsd: shut down NFSv4 state when nfsd_svc encounters an error
>   nfsd: just keep single lockd reference for nfsd
> 
>  fs/nfsd/nfs4state.c |    2 ++
>  fs/nfsd/nfsctl.c    |   20 ++++++++------------
>  fs/nfsd/nfssvc.c    |   36 +++++++++++++++++-------------------
>  3 files changed, 27 insertions(+), 31 deletions(-)
> 

Re: [PATCH 0/5] nfsd: fix error handling in write_ports interfaces (resend)

[Jeff Layton]

On Tue, 20 Jul 2010 10:55:40 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Jul 19, 2010 at 04:50:03PM -0400, Jeff Layton wrote:
> > This patchset is a resend of the set I sent a month or so ago, with
> > a couple of other patches to fix related problems that came to light
> > as we were discussing them.
> 
> So, rpc.nfsd starts the nfs server by more or less:
> 
> 	1. write nfs versions to /proc/fs/nfsd/versions
> 	2. write tcp socket fd to /proc/fs/nfsd/portlist
> 	3. write udp socket fd to /proc/fs/nfsd/portlist
> 	4. write number of threads to /proc/fs/nfsd/threads
> 
> A failure anywhere between step 2 and 4 leaves the server in an odd
> state: it's created, but not really running yet.
> 

Yep.

> Your patches help when it's step 2 that fails.  Probably that's the most
> common case, so good.
> 
> But what if we step 3 fails?  Or rpc.nfsd encounters some other error
> between steps 2 and 4?  Does it have any way to clean up?  (Is starting
> a single thread and then stopping it the only way to destroy the server
> at that point?)  And what if rpc.nfsd is interrupted before it gets to
> step 4?
> 

Then the server will remain "created", lockd will be up, etc and the
refcount will stay at 0. I think starting a single thread and stopping
it is the only way to clean it up.

> The whole interface seems strange and fragile.
> 
> The correct thing may be to apply your patches anyway, as long as they
> solve a problem and don't make things any worse.
> 
> But I fear we'll be spending more time on corner cases in the future.
> 
> As a first exercise, maybe we should figure out how the interface would
> look if we had the opportunity to start over from scratch.
> 

I wholeheartedly agree. This set is essentially a bandaid and I think
it helps the basic problems but there are plenty of others.

I'm all for redesigning the innards for better fault tolerance, but I
think we ought to shoot for not changing the userspace interface unless
there's just no other way, or it provides a substantial "win".

I think we could fix this in a much better way by not creating the
nfsd_serv until threads need to be started. 

Basically just have write_ports add socket info to a global list rather
than to the nfsd_serv itself. When nfsd_serv is created, we'd walk that
list and add those sockets to the nfsd_serv.

Any time that the portlist file is written to, we'd check to see if
nfsd is up. If it is then sync up the nfsd sockets with the contents of
the list.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/5] nfsd: fix error handling in write_ports interfaces (resend)

[J. Bruce Fields]

On Tue, Jul 20, 2010 at 11:43:13AM -0400, Jeff Layton wrote:
> On Tue, 20 Jul 2010 10:55:40 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Jul 19, 2010 at 04:50:03PM -0400, Jeff Layton wrote:
> > As a first exercise, maybe we should figure out how the interface would
> > look if we had the opportunity to start over from scratch.
> > 
> 
> I wholeheartedly agree. This set is essentially a bandaid and I think
> it helps the basic problems but there are plenty of others.
> 
> I'm all for redesigning the innards for better fault tolerance, but I
> think we ought to shoot for not changing the userspace interface unless
> there's just no other way, or it provides a substantial "win".

Yeah, agreed--that's why I said "as a first exercise".  Sometimes once
you figure out what you want, it also becomes easier to figure out how
to get there in a backwards-compatible way.

> I think we could fix this in a much better way by not creating the
> nfsd_serv until threads need to be started. 
> 
> Basically just have write_ports add socket info to a global list rather
> than to the nfsd_serv itself. When nfsd_serv is created, we'd walk that
> list and add those sockets to the nfsd_serv.
> 
> Any time that the portlist file is written to, we'd check to see if
> nfsd is up. If it is then sync up the nfsd sockets with the contents of
> the list.

If the problem with creating nfsd_server earlier is just that it
prevents things like versions or leasetime from being changed, perhaps
we should just relax that rule.

--b.

Re: [PATCH 0/5] nfsd: fix error handling in write_ports interfaces (resend)

[J. Bruce Fields]

On Tue, Jul 20, 2010 at 10:55:40AM -0400, J. Bruce Fields wrote:
> On Mon, Jul 19, 2010 at 04:50:03PM -0400, Jeff Layton wrote:
> > This patchset is a resend of the set I sent a month or so ago, with
> > a couple of other patches to fix related problems that came to light
> > as we were discussing them.
> 
> So, rpc.nfsd starts the nfs server by more or less:
> 
> 	1. write nfs versions to /proc/fs/nfsd/versions
> 	2. write tcp socket fd to /proc/fs/nfsd/portlist
> 	3. write udp socket fd to /proc/fs/nfsd/portlist
> 	4. write number of threads to /proc/fs/nfsd/threads
> 
> A failure anywhere between step 2 and 4 leaves the server in an odd
> state: it's created, but not really running yet.
> 
> Your patches help when it's step 2 that fails.  Probably that's the most
> common case, so good.
> 
> But what if we step 3 fails?  Or rpc.nfsd encounters some other error
> between steps 2 and 4?  Does it have any way to clean up?  (Is starting
> a single thread and then stopping it the only way to destroy the server
> at that point?)  And what if rpc.nfsd is interrupted before it gets to
> step 4?
> 
> The whole interface seems strange and fragile.

Also, looking at this from userland's point of view:

	- Why am I no longer allowed to set nfs versions after writing
	  to "portlist"?
	- Why does changing threads from non-zero to zero destroy
	  portlist configuration, but not any other configuration?

It's a strange interface to use.

Maybe it would be simpler (and solve your problem) if we made the rule
that any configuration can be done as long as nrthreads is 0.  And that
lockd startup also only happens on transition between zero and nonzero
nrthreads.  ?

(On the other hand, I don't really like overloading the meaning of "echo
0 >/proc/fs/nfsd/threads".  There might be times when it's convenient to
stall the server temporarily while still keeping nfsv4 state, sockets,
etc., all up.)

--b.