Skip to contentSkip to conversation

NFSD: simplify error paths in nfsd_svc()

PATCH In review [email protected] 3 participants 4 messages Aug 24, 2024 – Aug 27, 2024

Subsystems (3)
Stable branch NFS client NFS server
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

PATCH 1/1 NFSD: simplify error paths in nfsd_svc()

From: NeilBrown <[email protected]>

[ 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 <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfssvc.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)
Reported-by Li Lingfeng <[email protected]>
Suggested-by Li Lingfeng <[email protected]>
Tested-by Li Lingfeng <[email protected]>
fs/nfsd/nfssvc.c +4 -10
index 7911c4b3b5d3..710a54c7dffc 100644
@@ -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);
Signature · 1 line
2.45.1
#1 lore raw
Quoted 26 lines · On Sat, Aug 24, 2024 at 12:21:37PM -0400, [email protected] wrote:

From: NeilBrown <[email protected]>

[ 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 <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
Quoted 7 lines
---
 fs/nfsd/nfssvc.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)
Reported-by Li Lingfeng <[email protected]>
Suggested-by Li Lingfeng <[email protected]>
Tested-by Li Lingfeng <[email protected]>

Odd placement of these :)

Now queued up.

greg k-h

#2 lore raw
Quoted 35 lines · Greg KH
Quoted 30 lines · On Sat, Aug 24, 2024 at 12:21:37PM -0400, [email protected] wrote:

From: NeilBrown <[email protected]>

[ 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 <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>

---
fs/nfsd/nfssvc.c | 14 ++++----------

1 file changed, 4 insertions(+), 10 deletions(-)
Quoted 5 lines
Quoted 3 lines
Reported-by Li Lingfeng <[email protected]>
Suggested-by Li Lingfeng <[email protected]>
Tested-by Li Lingfeng <[email protected]>

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

#3 lore raw
Quoted 39 lines · Chuck Lever III
Quoted 39 lines · Greg KH
Quoted 33 lines · On Sat, Aug 24, 2024 at 12:21:37PM -0400, [email protected] wrote:

From: NeilBrown <[email protected]>

[ 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 <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>

---
fs/nfsd/nfssvc.c | 14 ++++----------

1 file changed, 4 insertions(+), 10 deletions(-)
Reported-by Li Lingfeng <[email protected]>
Suggested-by Li Lingfeng <[email protected]>
Tested-by Li Lingfeng <[email protected]>
Quoted 8 lines
Quoted 1 line

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!