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]>
---
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);
--
2.42.0
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 <[email protected]>
> ---
> 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 <[email protected]>
> On Sep 25, 2023, at 7:41 AM, Jeff Layton <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
Applied to nfsd-next.
--
Chuck Lever