* [PATCH 1/3] nfsd: don't try to shut down nfs4 state handling unless it's up
2010-06-07 15:33 [PATCH 0/3] nfsd: fix error handling in write_ports interfaces Jeff Layton
@ 2010-06-07 15:33 ` Jeff Layton
2010-06-08 23:58 ` J. Bruce Fields
2010-06-26 15:53 ` J. Bruce Fields
2010-06-07 15:33 ` [PATCH 2/3] nfsd: fix error handling when starting nfsd with rpcbind down Jeff Layton
` (3 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Jeff Layton @ 2010-06-07 15:33 UTC (permalink / raw)
To: bfields; +Cc: linux-nfs
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/3] nfsd: don't try to shut down nfs4 state handling unless it's up
2010-06-07 15:33 ` [PATCH 1/3] nfsd: don't try to shut down nfs4 state handling unless it's up Jeff Layton
@ 2010-06-08 23:58 ` J. Bruce Fields
2010-06-09 10:29 ` Jeff Layton
2010-06-26 15:53 ` J. Bruce Fields
1 sibling, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2010-06-08 23:58 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-nfs
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/3] nfsd: don't try to shut down nfs4 state handling unless it's up
2010-06-08 23:58 ` J. Bruce Fields
@ 2010-06-09 10:29 ` Jeff Layton
[not found] ` <20100609062922.4bae21ac-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Jeff Layton @ 2010-06-09 10:29 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] nfsd: don't try to shut down nfs4 state handling unless it's up
2010-06-07 15:33 ` [PATCH 1/3] nfsd: don't try to shut down nfs4 state handling unless it's up Jeff Layton
2010-06-08 23:58 ` J. Bruce Fields
@ 2010-06-26 15:53 ` J. Bruce Fields
[not found] ` <20100626155351.GA16951-+qGSg9AQ1cLTsXDwO4sDpg@public.gmane.org>
1 sibling, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2010-06-26 15:53 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-nfs
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] nfsd: fix error handling when starting nfsd with rpcbind down
2010-06-07 15:33 [PATCH 0/3] nfsd: fix error handling in write_ports interfaces Jeff Layton
2010-06-07 15:33 ` [PATCH 1/3] nfsd: don't try to shut down nfs4 state handling unless it's up Jeff Layton
@ 2010-06-07 15:33 ` Jeff Layton
2010-06-07 15:33 ` [PATCH 3/3] nfsd: fix error handling in __write_ports_addxprt Jeff Layton
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Jeff Layton @ 2010-06-07 15:33 UTC (permalink / raw)
To: bfields; +Cc: linux-nfs
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 3/3] nfsd: fix error handling in __write_ports_addxprt
2010-06-07 15:33 [PATCH 0/3] nfsd: fix error handling in write_ports interfaces Jeff Layton
2010-06-07 15:33 ` [PATCH 1/3] nfsd: don't try to shut down nfs4 state handling unless it's up Jeff Layton
2010-06-07 15:33 ` [PATCH 2/3] nfsd: fix error handling when starting nfsd with rpcbind down Jeff Layton
@ 2010-06-07 15:33 ` Jeff Layton
2010-06-09 0:00 ` [PATCH 0/3] nfsd: fix error handling in write_ports interfaces J. Bruce Fields
2010-06-09 0:49 ` J. Bruce Fields
4 siblings, 0 replies; 17+ messages in thread
From: Jeff Layton @ 2010-06-07 15:33 UTC (permalink / raw)
To: bfields; +Cc: linux-nfs
__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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] nfsd: fix error handling in write_ports interfaces
2010-06-07 15:33 [PATCH 0/3] nfsd: fix error handling in write_ports interfaces Jeff Layton
` (2 preceding siblings ...)
2010-06-07 15:33 ` [PATCH 3/3] nfsd: fix error handling in __write_ports_addxprt Jeff Layton
@ 2010-06-09 0:00 ` J. Bruce Fields
2010-06-09 10:43 ` Jeff Layton
2010-06-09 0:49 ` J. Bruce Fields
4 siblings, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2010-06-09 0:00 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-nfs
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(-)
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 0/3] nfsd: fix error handling in write_ports interfaces
2010-06-09 0:00 ` [PATCH 0/3] nfsd: fix error handling in write_ports interfaces J. Bruce Fields
@ 2010-06-09 10:43 ` Jeff Layton
0 siblings, 0 replies; 17+ messages in thread
From: Jeff Layton @ 2010-06-09 10:43 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] nfsd: fix error handling in write_ports interfaces
2010-06-07 15:33 [PATCH 0/3] nfsd: fix error handling in write_ports interfaces Jeff Layton
` (3 preceding siblings ...)
2010-06-09 0:00 ` [PATCH 0/3] nfsd: fix error handling in write_ports interfaces J. Bruce Fields
@ 2010-06-09 0:49 ` J. Bruce Fields
2010-06-09 10:55 ` Jeff Layton
4 siblings, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2010-06-09 0:49 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-nfs
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(-)
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 0/3] nfsd: fix error handling in write_ports interfaces
2010-06-09 0:49 ` J. Bruce Fields
@ 2010-06-09 10:55 ` Jeff Layton
0 siblings, 0 replies; 17+ messages in thread
From: Jeff Layton @ 2010-06-09 10:55 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
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>
^ permalink raw reply [flat|nested] 17+ messages in thread