[PATCH 6.6.y] NFSD: simplify error paths in nfsd_svc()

[PATCH 6.6.y] NFSD: simplify error paths in nfsd_svc()

[cel]

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>

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);
-- 
2.45.1

Re: [PATCH 6.6.y] NFSD: simplify error paths in nfsd_svc()

[Greg KH]

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

Re: [PATCH 6.6.y] NFSD: simplify error paths in nfsd_svc()

[Chuck Lever III]

> 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

Re: [PATCH 6.6.y] NFSD: simplify error paths in nfsd_svc()

[Greg KH]

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!