| Message ID | 169561203735.19404.6014131036692240448@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [nfsd-next] NFSD: simplify error paths in nfsd_svc() | expand |
On Mon, 2023-09-25 at 12:06 +1000, NeilBrown wrote: > The error paths in nfsd_svc() are needlessly complex and can result in a > final call to svc_put() without nfsd_last_thread() being called. This > results in the listening sockets not being closed properly. > > The per-netns setup provided by nfsd_startup_new() and removed by > nfsd_shutdown_net() is needed precisely when there are running threads. > So we don't need nfsd_up_before. We don't need to know if it *was* up. > We only need to know if any threads are left. If none are, then we must > call nfsd_shutdown_net(). But we don't need to do that explicitly as > nfsd_last_thread() does that for us. > > So simply call nfsd_last_thread() before the last svc_put() if there are > no running threads. That will always do the right thing. > > Also discard: > pr_info("nfsd: last server has exited, flushing export cache\n"); > It may not be true if an attempt to start the first server failed, and > it isn't particularly helpful and it simply reports normal behaviour. > Thanks. Removing that is long overdue. > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfssvc.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c > index c5890cdfe97b..d6122bb2d167 100644 > --- a/fs/nfsd/nfssvc.c > +++ b/fs/nfsd/nfssvc.c > @@ -572,7 +572,6 @@ static void nfsd_last_thread(struct net *net) > return; > > nfsd_shutdown_net(net); > - pr_info("nfsd: last server has exited, flushing export cache\n"); > nfsd_export_flush(net); > } > > @@ -786,7 +785,6 @@ int > nfsd_svc(int nrservs, struct net *net, const struct cred *cred) > { > int error; > - bool nfsd_up_before; > struct nfsd_net *nn = net_generic(net, nfsd_net_id); > struct svc_serv *serv; > > @@ -806,8 +804,6 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred) > error = nfsd_create_serv(net); > if (error) > goto out; > - > - nfsd_up_before = nn->nfsd_net_up; > serv = nn->nfsd_serv; > > error = nfsd_startup_net(net, cred); > @@ -815,17 +811,15 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred) > goto out_put; > error = svc_set_num_threads(serv, NULL, nrservs); > if (error) > - goto out_shutdown; > + goto out_put; > error = serv->sv_nrthreads; > - if (error == 0) > - nfsd_last_thread(net); > -out_shutdown: > - if (error < 0 && !nfsd_up_before) > - nfsd_shutdown_net(net); > out_put: > /* Threads now hold service active */ > if (xchg(&nn->keep_active, 0)) > svc_put(serv); > + > + if (serv->sv_nrthreads == 0) > + nfsd_last_thread(net); > svc_put(serv); > out: > mutex_unlock(&nfsd_mutex); Nice cleanup. Reviewed-by: Jeff Layton <jlayton@kernel.org>
> On Sep 25, 2023, at 7:41 AM, Jeff Layton <jlayton@kernel.org> wrote: > > On Mon, 2023-09-25 at 12:06 +1000, NeilBrown wrote: >> The error paths in nfsd_svc() are needlessly complex and can result in a >> final call to svc_put() without nfsd_last_thread() being called. This >> results in the listening sockets not being closed properly. >> >> The per-netns setup provided by nfsd_startup_new() and removed by >> nfsd_shutdown_net() is needed precisely when there are running threads. >> So we don't need nfsd_up_before. We don't need to know if it *was* up. >> We only need to know if any threads are left. If none are, then we must >> call nfsd_shutdown_net(). But we don't need to do that explicitly as >> nfsd_last_thread() does that for us. >> >> So simply call nfsd_last_thread() before the last svc_put() if there are >> no running threads. That will always do the right thing. >> >> Also discard: >> pr_info("nfsd: last server has exited, flushing export cache\n"); >> It may not be true if an attempt to start the first server failed, and >> it isn't particularly helpful and it simply reports normal behaviour. >> > > Thanks. Removing that is long overdue. > >> Signed-off-by: NeilBrown <neilb@suse.de> >> --- >> fs/nfsd/nfssvc.c | 14 ++++---------- >> 1 file changed, 4 insertions(+), 10 deletions(-) >> >> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c >> index c5890cdfe97b..d6122bb2d167 100644 >> --- a/fs/nfsd/nfssvc.c >> +++ b/fs/nfsd/nfssvc.c >> @@ -572,7 +572,6 @@ static void nfsd_last_thread(struct net *net) >> return; >> >> nfsd_shutdown_net(net); >> - pr_info("nfsd: last server has exited, flushing export cache\n"); >> nfsd_export_flush(net); >> } >> >> @@ -786,7 +785,6 @@ int >> nfsd_svc(int nrservs, struct net *net, const struct cred *cred) >> { >> int error; >> - bool nfsd_up_before; >> struct nfsd_net *nn = net_generic(net, nfsd_net_id); >> struct svc_serv *serv; >> >> @@ -806,8 +804,6 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred) >> error = nfsd_create_serv(net); >> if (error) >> goto out; >> - >> - nfsd_up_before = nn->nfsd_net_up; >> serv = nn->nfsd_serv; >> >> error = nfsd_startup_net(net, cred); >> @@ -815,17 +811,15 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred) >> goto out_put; >> error = svc_set_num_threads(serv, NULL, nrservs); >> if (error) >> - goto out_shutdown; >> + goto out_put; >> error = serv->sv_nrthreads; >> - if (error == 0) >> - nfsd_last_thread(net); >> -out_shutdown: >> - if (error < 0 && !nfsd_up_before) >> - nfsd_shutdown_net(net); >> out_put: >> /* Threads now hold service active */ >> if (xchg(&nn->keep_active, 0)) >> svc_put(serv); >> + >> + if (serv->sv_nrthreads == 0) >> + nfsd_last_thread(net); >> svc_put(serv); >> out: >> mutex_unlock(&nfsd_mutex); > > Nice cleanup. > > Reviewed-by: Jeff Layton <jlayton@kernel.org> Applied to nfsd-next. -- Chuck Lever
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index c5890cdfe97b..d6122bb2d167 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -572,7 +572,6 @@ static void nfsd_last_thread(struct net *net) return; nfsd_shutdown_net(net); - pr_info("nfsd: last server has exited, flushing export cache\n"); nfsd_export_flush(net); } @@ -786,7 +785,6 @@ int nfsd_svc(int nrservs, struct net *net, const struct cred *cred) { int error; - bool nfsd_up_before; struct nfsd_net *nn = net_generic(net, nfsd_net_id); struct svc_serv *serv; @@ -806,8 +804,6 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred) error = nfsd_create_serv(net); if (error) goto out; - - nfsd_up_before = nn->nfsd_net_up; serv = nn->nfsd_serv; error = nfsd_startup_net(net, cred); @@ -815,17 +811,15 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred) goto out_put; error = svc_set_num_threads(serv, NULL, nrservs); if (error) - goto out_shutdown; + goto out_put; error = serv->sv_nrthreads; - if (error == 0) - nfsd_last_thread(net); -out_shutdown: - if (error < 0 && !nfsd_up_before) - nfsd_shutdown_net(net); out_put: /* Threads now hold service active */ if (xchg(&nn->keep_active, 0)) svc_put(serv); + + if (serv->sv_nrthreads == 0) + nfsd_last_thread(net); svc_put(serv); out: mutex_unlock(&nfsd_mutex);
The error paths in nfsd_svc() are needlessly complex and can result in a final call to svc_put() without nfsd_last_thread() being called. This results in the listening sockets not being closed properly. The per-netns setup provided by nfsd_startup_new() and removed by nfsd_shutdown_net() is needed precisely when there are running threads. So we don't need nfsd_up_before. We don't need to know if it *was* up. We only need to know if any threads are left. If none are, then we must call nfsd_shutdown_net(). But we don't need to do that explicitly as nfsd_last_thread() does that for us. So simply call nfsd_last_thread() before the last svc_put() if there are no running threads. That will always do the right thing. Also discard: pr_info("nfsd: last server has exited, flushing export cache\n"); It may not be true if an attempt to start the first server failed, and it isn't particularly helpful and it simply reports normal behaviour. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfssvc.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)