[PATCH 0/3] nfsd: fix error handling in write_ports interfaces

[PATCH 0/3] nfsd: fix error handling in write_ports interfaces

[Jeff Layton]

This patchset fixes some problems with refcounting when there are
problems starting up nfsd. The easiest way to reproduce this is to have
rpcbind down and then try to start nfsd. The write_ports calls will
generally return failure at that point due to the fact that lockd can't
register its ports. That leaves the nfsd_serv pointer set, with the
sv_threads count set at 0. The first two patches fix this problem.

The third patch, I'm not 100% sure on. It looks correct to me, but the
intent of the existing code is not very clear. I know this interface is
used by the rdma code, and I may be missing the point of doing it this
way. If the existing code is correct as-is, I'll plan to do a patch to
add some clarifying comments.

It also seems suspicious to me that __write_ports_addfd/delfd take and
put lockd references, but the addxprt/delxprt interfaces do not. If
someone were to addfd a socket and then delxprt it, it'll leave a lockd
reference dangling.

Should we be taking and putting lockd references in those codepaths as
well?

Jeff Layton (3):
  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

 fs/nfsd/nfs4state.c |    2 ++
 fs/nfsd/nfsctl.c    |   18 ++++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

[PATCH 1/3] 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 1176708..fc52920 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4124,6 +4124,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

[PATCH 2/3] 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/3] 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

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

[J. Bruce Fields]

On Mon, Jun 07, 2010 at 11:33:18AM -0400, Jeff Layton wrote:
> If someone tries to shut down the laundry_wq while it isn't up it'll
> cause an oops.

OK, thanks.

Hm: what about the opposite (admittedly probably less crucial) problem:
are there cases where nfs4_state_start() gets called but never
nfs4_state_shutdown?

>From a quick look, I don't see what prevents that.

--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 1176708..fc52920 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4124,6 +4124,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 0/3] nfsd: fix error handling in write_ports interfaces

[J. Bruce Fields]

On Mon, Jun 07, 2010 at 11:33:17AM -0400, Jeff Layton wrote:
> This patchset fixes some problems with refcounting when there are
> problems starting up nfsd. The easiest way to reproduce this is to have
> rpcbind down and then try to start nfsd. The write_ports calls will
> generally return failure at that point due to the fact that lockd can't
> register its ports. That leaves the nfsd_serv pointer set, with the
> sv_threads count set at 0. The first two patches fix this problem.

Does this look like it's always been a problem, or was it introduced by
recent changes?

(Just a question of priority and whether it should be fixed in -stable
branches too.)

--b.

> 
> The third patch, I'm not 100% sure on. It looks correct to me, but the
> intent of the existing code is not very clear. I know this interface is
> used by the rdma code, and I may be missing the point of doing it this
> way. If the existing code is correct as-is, I'll plan to do a patch to
> add some clarifying comments.
> 
> It also seems suspicious to me that __write_ports_addfd/delfd take and
> put lockd references, but the addxprt/delxprt interfaces do not. If
> someone were to addfd a socket and then delxprt it, it'll leave a lockd
> reference dangling.
> 
> Should we be taking and putting lockd references in those codepaths as
> well?
> 
> Jeff Layton (3):
>   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
> 
>  fs/nfsd/nfs4state.c |    2 ++
>  fs/nfsd/nfsctl.c    |   18 ++++++++++++------
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 

Re: [PATCH 0/3] nfsd: fix error handling in write_ports interfaces

[J. Bruce Fields]

On Mon, Jun 07, 2010 at 11:33:17AM -0400, Jeff Layton wrote:
> This patchset fixes some problems with refcounting when there are
> problems starting up nfsd. The easiest way to reproduce this is to have
> rpcbind down and then try to start nfsd. The write_ports calls will
> generally return failure at that point due to the fact that lockd can't
> register its ports. That leaves the nfsd_serv pointer set, with the
> sv_threads count set at 0. The first two patches fix this problem.
> 
> The third patch, I'm not 100% sure on. It looks correct to me, but the
> intent of the existing code is not very clear. I know this interface is
> used by the rdma code, and I may be missing the point of doing it this
> way. If the existing code is correct as-is, I'll plan to do a patch to
> add some clarifying comments.
> 
> It also seems suspicious to me that __write_ports_addfd/delfd take and
> put lockd references, but the addxprt/delxprt interfaces do not. If
> someone were to addfd a socket and then delxprt it, it'll leave a lockd
> reference dangling.

I haven't looked at this closely enough to have any though beyond: boy,
it seems confusing to use sv_nrthreads this way.  What is the
refcounting actually meant to accomplish, and if we were designing it
from scratch, what would we do?

--b.

> 
> Should we be taking and putting lockd references in those codepaths as
> well?
> 
> Jeff Layton (3):
>   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
> 
>  fs/nfsd/nfs4state.c |    2 ++
>  fs/nfsd/nfsctl.c    |   18 ++++++++++++------
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 

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

[Jeff Layton]

On Tue, 8 Jun 2010 19:58:28 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Jun 07, 2010 at 11:33:18AM -0400, Jeff Layton wrote:
> > If someone tries to shut down the laundry_wq while it isn't up it'll
> > cause an oops.
> 
> OK, thanks.
> 
> Hm: what about the opposite (admittedly probably less crucial) problem:
> are there cases where nfs4_state_start() gets called but never
> nfs4_state_shutdown?
> 
> From a quick look, I don't see what prevents that.
> 
> --b.
> 

I don't see that problem. nfs4_state_shutdown gets called when the
nfsd_serv is torn down, and it only gets brought up in nfsd_svc, so as
far as I can tell it'll always be up as long as there are nfsd kthreads
running.

I should make it clear that this patch is just a prerequisite for the
following patches. I don't know of a way to trigger this oops with the
existing code, but if we call svc_destroy from the write_ports
codepaths like I'm proposing then it can happen.

> > 
> > 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 1176708..fc52920 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4124,6 +4124,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
> > 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/3] nfsd: fix error handling in write_ports interfaces

[Jeff Layton]

On Tue, 8 Jun 2010 20:00:03 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Jun 07, 2010 at 11:33:17AM -0400, Jeff Layton wrote:
> > This patchset fixes some problems with refcounting when there are
> > problems starting up nfsd. The easiest way to reproduce this is to have
> > rpcbind down and then try to start nfsd. The write_ports calls will
> > generally return failure at that point due to the fact that lockd can't
> > register its ports. That leaves the nfsd_serv pointer set, with the
> > sv_threads count set at 0. The first two patches fix this problem.
> 
> Does this look like it's always been a problem, or was it introduced by
> recent changes?
> 
> (Just a question of priority and whether it should be fixed in -stable
> branches too.)
> 

I think it's a long-standing bug -- at least since 2006 or so when the
portlist file was added, but some recent changes made it easier to hit.

Our QA group has a test where they restart both the nfs "service" and
rpcbind. With the recent change to using TCP to do rpcbind
registrations, the kernel now can hold open a socket to rpcbind for a
little while after doing the registration.

If you restart rpcbind within that window, it can fail to bind to port
111 as it didn't use SO_REUSEADDR. I recently proposed a patch to
rpcbind to fix that:

http://sourceforge.net/mailarchive/forum.php?thread_name=1275575657-9666-1-git-send-email-jlayton%40redhat.com&forum_name=libtirpc-devel

...portmap has a similar bug, but I haven't gotten around to fixing it
there yet.

Due to that problem, our QA group ended up trying to start nfsd with
rpcbind non-functional. When they got rpcbind to start, then they still
couldn't bring up nfsd immediately since nfsd_serv had already been
created and write_versions failed. 

IMO, this set probably isn't stable material. It's a nuisance, but the
simple workaround is to just run "rpc.nfsd 0" and then you can start up
nfsd.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/3] nfsd: fix error handling in write_ports interfaces

[Jeff Layton]

On Tue, 8 Jun 2010 20:49:58 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Jun 07, 2010 at 11:33:17AM -0400, Jeff Layton wrote:
> > This patchset fixes some problems with refcounting when there are
> > problems starting up nfsd. The easiest way to reproduce this is to have
> > rpcbind down and then try to start nfsd. The write_ports calls will
> > generally return failure at that point due to the fact that lockd can't
> > register its ports. That leaves the nfsd_serv pointer set, with the
> > sv_threads count set at 0. The first two patches fix this problem.
> > 
> > The third patch, I'm not 100% sure on. It looks correct to me, but the
> > intent of the existing code is not very clear. I know this interface is
> > used by the rdma code, and I may be missing the point of doing it this
> > way. If the existing code is correct as-is, I'll plan to do a patch to
> > add some clarifying comments.
> > 
> > It also seems suspicious to me that __write_ports_addfd/delfd take and
> > put lockd references, but the addxprt/delxprt interfaces do not. If
> > someone were to addfd a socket and then delxprt it, it'll leave a lockd
> > reference dangling.
> 
> I haven't looked at this closely enough to have any though beyond: boy,
> it seems confusing to use sv_nrthreads this way.  What is the
> refcounting actually meant to accomplish, and if we were designing it
> from scratch, what would we do?
> 

Yeah, I had the same thought. The fundamental problem I think is that
the refcounting serves two purposes:

1) it tracks the number of running nfsd threads

2) it serves as a refcount for the nfsd_serv itself

Of course, neither of these is entirely accurate since they're two
different things...

I'm not sure how to make this better. One idea might be to store the
"portlist" information in a separate container that's divorced from
the nfsd_serv. Then when you start the first thread you'd just bring up
whatever sockets were in the container. If you write to the portlist
file then you'd just sync up the sockets that nfsd_serv has open with
what's in the container.

-- 
Jeff Layton <jlayton@redhat.com>

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

[J. Bruce Fields]

On Wed, Jun 09, 2010 at 06:29:22AM -0400, Jeff Layton wrote:
> On Tue, 8 Jun 2010 19:58:28 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Jun 07, 2010 at 11:33:18AM -0400, Jeff Layton wrote:
> > > If someone tries to shut down the laundry_wq while it isn't up it'll
> > > cause an oops.
> > 
> > OK, thanks.
> > 
> > Hm: what about the opposite (admittedly probably less crucial) problem:
> > are there cases where nfs4_state_start() gets called but never
> > nfs4_state_shutdown?
> > 
> > From a quick look, I don't see what prevents that.
> > 
> > --b.
> > 
> 
> I don't see that problem. nfs4_state_shutdown gets called when the
> nfsd_serv is torn down, and it only gets brought up in nfsd_svc,

If nfs4_state_start() succeeds, then nfsd_create_server() is called.  It calls
svc_create_pooled(), which is passed nfsd_last_thread() as a parameter.

If svc_create_pooled succeeds, then I think we're guaranteed nfsd_last_thread()
will get called eventually.

But if svc_create_pooled fails, I don't think nfsd_last_thread() is ever
called.

--b.

> so as
> far as I can tell it'll always be up as long as there are nfsd kthreads
> running.
> 
> I should make it clear that this patch is just a prerequisite for the
> following patches. I don't know of a way to trigger this oops with the
> existing code, but if we call svc_destroy from the write_ports
> codepaths like I'm proposing then it can happen.
> 
> > > 
> > > 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 1176708..fc52920 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -4124,6 +4124,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
> > > 
> 
> 
> -- 
> Jeff Layton <jlayton@redhat.com>

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

[Jeff Layton]

On Wed, 9 Jun 2010 14:09:07 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jun 09, 2010 at 06:29:22AM -0400, Jeff Layton wrote:
> > On Tue, 8 Jun 2010 19:58:28 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Mon, Jun 07, 2010 at 11:33:18AM -0400, Jeff Layton wrote:
> > > > If someone tries to shut down the laundry_wq while it isn't up it'll
> > > > cause an oops.
> > > 
> > > OK, thanks.
> > > 
> > > Hm: what about the opposite (admittedly probably less crucial) problem:
> > > are there cases where nfs4_state_start() gets called but never
> > > nfs4_state_shutdown?
> > > 
> > > From a quick look, I don't see what prevents that.
> > > 
> > > --b.
> > > 
> > 
> > I don't see that problem. nfs4_state_shutdown gets called when the
> > nfsd_serv is torn down, and it only gets brought up in nfsd_svc,
> 
> If nfs4_state_start() succeeds, then nfsd_create_server() is called.  It calls
> svc_create_pooled(), which is passed nfsd_last_thread() as a parameter.
> 
> If svc_create_pooled succeeds, then I think we're guaranteed nfsd_last_thread()
> will get called eventually.
> 
> But if svc_create_pooled fails, I don't think nfsd_last_thread() is ever
> called.
> 

Ahh ok, I see what you're talking about now. Yeah, that should probably
also be fixed. I think we just need to ensure to shut down the state in
an error condition. Something like this compile-tested only patch?

>From 19bd25ea9a76ea184e4c30831765e812ce19b0a5 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
Date: Wed, 9 Jun 2010 14:21:51 -0400
Subject: [PATCH] nfsd: shut down NFSv4 state when nfsd_svc encounters an error

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

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

[J. Bruce Fields]

On Wed, Jun 09, 2010 at 02:29:43PM -0400, Jeff Layton wrote:
> Ahh ok, I see what you're talking about now. Yeah, that should probably
> also be fixed. I think we just need to ensure to shut down the state in
> an error condition. Something like this compile-tested only patch?
> 
> From 19bd25ea9a76ea184e4c30831765e812ce19b0a5 Mon Sep 17 00:00:00 2001
> From: Jeff Layton <jlayton@redhat.com>
> Date: Wed, 9 Jun 2010 14:21:51 -0400
> Subject: [PATCH] nfsd: shut down NFSv4 state when nfsd_svc encounters an error
> 
> 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).

The patch looks right to me, thanks.

--b.

> 
> 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
> 

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

[Jeff Layton]

On Sun, 13 Jun 2010 16:28:42 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jun 09, 2010 at 02:29:43PM -0400, Jeff Layton wrote:
> > Ahh ok, I see what you're talking about now. Yeah, that should probably
> > also be fixed. I think we just need to ensure to shut down the state in
> > an error condition. Something like this compile-tested only patch?
> > 
> > From 19bd25ea9a76ea184e4c30831765e812ce19b0a5 Mon Sep 17 00:00:00 2001
> > From: Jeff Layton <jlayton@redhat.com>
> > Date: Wed, 9 Jun 2010 14:21:51 -0400
> > Subject: [PATCH] nfsd: shut down NFSv4 state when nfsd_svc encounters an error
> > 
> > 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).
> 
> The patch looks right to me, thanks.
> 
> --b.
> 

FWIW, I've done a bit of testing with this patch on top of the other
set and it seems to be ok. That said, I don't have a reliable test that
makes the state setup "leak" like this, so I can't really say much
other than that it seems to be ok.

> > 
> > 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
> > 


-- 
Jeff Layton <jlayton@redhat.com>

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

[J. Bruce Fields]

On Tue, Jun 15, 2010 at 01:36:22PM -0400, Jeff Layton wrote:
> On Sun, 13 Jun 2010 16:28:42 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Wed, Jun 09, 2010 at 02:29:43PM -0400, Jeff Layton wrote:
> > > Ahh ok, I see what you're talking about now. Yeah, that should probably
> > > also be fixed. I think we just need to ensure to shut down the state in
> > > an error condition. Something like this compile-tested only patch?
> > > 
> > > From 19bd25ea9a76ea184e4c30831765e812ce19b0a5 Mon Sep 17 00:00:00 2001
> > > From: Jeff Layton <jlayton@redhat.com>
> > > Date: Wed, 9 Jun 2010 14:21:51 -0400
> > > Subject: [PATCH] nfsd: shut down NFSv4 state when nfsd_svc encounters an error
> > > 
> > > 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).
> > 
> > The patch looks right to me, thanks.
> > 
> > --b.
> > 
> 
> FWIW, I've done a bit of testing with this patch on top of the other
> set and it seems to be ok. That said, I don't have a reliable test that
> makes the state setup "leak" like this, so I can't really say much
> other than that it seems to be ok.

OK, thanks.  It should show up in my 2.6.36 tree sometime soon.

--b.

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

[J. Bruce Fields]

On Mon, Jun 07, 2010 at 11:33:18AM -0400, Jeff Layton wrote:
> If someone tries to shut down the laundry_wq while it isn't up it'll
> cause an oops.

Sorry for getting behind....

I'm not seeing how this can happen: nfs4_state_shutdown() is called from
nfsd_last_thread(), which can't be called until after nfsd_create_serv()
is called.  But nfsd_create_serv() is called after nfs4_state_start().

Oh, I see, I'm overlooking __write_ports_add{fd,xprt}(), which can call
nfsd_create_serv() without first calling nfs4_state_start().  Argh.

--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 1176708..fc52920 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4124,6 +4124,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/3] nfsd: don't try to shut down nfs4 state handling unless it's up

[Jeff Layton]

On Sat, 26 Jun 2010 11:53:51 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Jun 07, 2010 at 11:33:18AM -0400, Jeff Layton wrote:
> > If someone tries to shut down the laundry_wq while it isn't up it'll
> > cause an oops.
> 
> Sorry for getting behind....
> 

No huge hurry. This series is pretty much 2.6.36 material unless you
see a reason to push them sooner.

> I'm not seeing how this can happen: nfs4_state_shutdown() is called from
> nfsd_last_thread(), which can't be called until after nfsd_create_serv()
> is called.  But nfsd_create_serv() is called after nfs4_state_start().
> 
> Oh, I see, I'm overlooking __write_ports_add{fd,xprt}(), which can call
> nfsd_create_serv() without first calling nfs4_state_start().  Argh.
> 

Yep...and rpc.nfsd does write fd's to the kernel before bringing up
threads, so this situation is actually the norm. The later patches in
this series also mandate this to prevent oopses when there are errors
bringing up the service.

-- 
Jeff Layton <jlayton@redhat.com>