| Message ID | 20240824162137.2157-1-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [6.6.y] NFSD: simplify error paths in nfsd_svc() | expand |
On Sat, Aug 24, 2024 at 12:21:37PM -0400, cel@kernel.org wrote: > From: NeilBrown <neilb@suse.de> > > [ Upstream commit bf32075256e9dd9c6b736859e2c5813981339908 ] > > 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> > Reviewed-by: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfssvc.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > Reported-by: Li Lingfeng <lilingfeng3@huawei.com> > Suggested-by: Li Lingfeng <lilingfeng3@huawei.com> > Tested-by: Li Lingfeng <lilingfeng3@huawei.com> Odd placement of these :) Now queued up. greg k-h
> On Aug 27, 2024, at 8:46 AM, Greg KH <greg@kroah.com> wrote: > > On Sat, Aug 24, 2024 at 12:21:37PM -0400, cel@kernel.org wrote: >> From: NeilBrown <neilb@suse.de> >> >> [ Upstream commit bf32075256e9dd9c6b736859e2c5813981339908 ] >> >> 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> >> Reviewed-by: Jeff Layton <jlayton@kernel.org> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> fs/nfsd/nfssvc.c | 14 ++++---------- >> 1 file changed, 4 insertions(+), 10 deletions(-) >> >> Reported-by: Li Lingfeng <lilingfeng3@huawei.com> >> Suggested-by: Li Lingfeng <lilingfeng3@huawei.com> >> Tested-by: Li Lingfeng <lilingfeng3@huawei.com> > > Odd placement of these :) Wasn't sure I was supposed to add them to the actual patch description because they weren't part of the original upstream commit. But if that's OK to do for stable patches, I will add them in the usual spot next time. -- Chuck Lever
On Tue, Aug 27, 2024 at 02:25:03PM +0000, Chuck Lever III wrote: > > > > On Aug 27, 2024, at 8:46 AM, Greg KH <greg@kroah.com> wrote: > > > > On Sat, Aug 24, 2024 at 12:21:37PM -0400, cel@kernel.org wrote: > >> From: NeilBrown <neilb@suse.de> > >> > >> [ Upstream commit bf32075256e9dd9c6b736859e2c5813981339908 ] > >> > >> 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> > >> Reviewed-by: Jeff Layton <jlayton@kernel.org> > >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > >> --- > >> fs/nfsd/nfssvc.c | 14 ++++---------- > >> 1 file changed, 4 insertions(+), 10 deletions(-) > >> > >> Reported-by: Li Lingfeng <lilingfeng3@huawei.com> > >> Suggested-by: Li Lingfeng <lilingfeng3@huawei.com> > >> Tested-by: Li Lingfeng <lilingfeng3@huawei.com> > > > > Odd placement of these :) > > Wasn't sure I was supposed to add them to the actual > patch description because they weren't part of the > original upstream commit. > > But if that's OK to do for stable patches, I will > add them in the usual spot next time. Yes please, thanks!
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index 7911c4b3b5d3..710a54c7dffc 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -567,7 +567,6 @@ 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); } @@ -783,7 +782,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; @@ -803,8 +801,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); @@ -812,17 +808,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);