We are required to hold a reference to the svc_serv struct while
stopping the last thread, as doing that could otherwise drop the last
reference itself and the svc_serv would be freed while still in use.
lockd doesn't do this. After startup, the only reference is held by the
running thread.
So change locked to hold a reference on nlmsvc_serv while-ever the
service is active, and only drop it after the last thread has been
stopped.
Note: it doesn't really make sense for threads to hold references to the
svc_serv any more. The fact threads are included in serv->sv_nrthreads
is sufficient. Maybe a future patch could address this.
Reported-by: Jeff Layton <[email protected]>
Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
Signed-off-by: NeilBrown <[email protected]>
---
Thanks for the report Jeff !!!
fs/lockd/svc.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index b441c706c2b8..7a5c90a00522 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -345,10 +345,10 @@ static int lockd_get(void)
serv->sv_maxconn = nlm_max_connections;
error = svc_set_num_threads(serv, NULL, 1);
- /* The thread now holds the only reference */
- svc_put(serv);
- if (error < 0)
+ if (error < 0) {
+ svc_put(serv);
return error;
+ }
nlmsvc_serv = serv;
register_inetaddr_notifier(&lockd_inetaddr_notifier);
@@ -374,6 +374,7 @@ static void lockd_put(void)
svc_set_num_threads(nlmsvc_serv, NULL, 0);
timer_delete_sync(&nlmsvc_retry);
+ svc_put(nlmsvc_serv);
nlmsvc_serv = NULL;
dprintk("lockd_down: service destroyed\n");
}
--
2.42.0
On Tue, 2023-10-10 at 10:35 +1100, NeilBrown wrote:
> We are required to hold a reference to the svc_serv struct while
> stopping the last thread, as doing that could otherwise drop the last
> reference itself and the svc_serv would be freed while still in use.
>
> lockd doesn't do this. After startup, the only reference is held by the
> running thread.
>
> So change locked to hold a reference on nlmsvc_serv while-ever the
> service is active, and only drop it after the last thread has been
> stopped.
>
> Note: it doesn't really make sense for threads to hold references to the
> svc_serv any more. The fact threads are included in serv->sv_nrthreads
> is sufficient. Maybe a future patch could address this.
>
> Reported-by: Jeff Layton <[email protected]>
> Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
> Signed-off-by: NeilBrown <[email protected]>
> ---
>
> Thanks for the report Jeff !!!
>
> fs/lockd/svc.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index b441c706c2b8..7a5c90a00522 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -345,10 +345,10 @@ static int lockd_get(void)
>
> serv->sv_maxconn = nlm_max_connections;
> error = svc_set_num_threads(serv, NULL, 1);
> - /* The thread now holds the only reference */
> - svc_put(serv);
> - if (error < 0)
> + if (error < 0) {
> + svc_put(serv);
> return error;
> + }
>
> nlmsvc_serv = serv;
> register_inetaddr_notifier(&lockd_inetaddr_notifier);
> @@ -374,6 +374,7 @@ static void lockd_put(void)
>
> svc_set_num_threads(nlmsvc_serv, NULL, 0);
> timer_delete_sync(&nlmsvc_retry);
> + svc_put(nlmsvc_serv);
> nlmsvc_serv = NULL;
> dprintk("lockd_down: service destroyed\n");
> }
Thanks Neil! You can add:
Reviewed-by: Jeff Layton <[email protected]>
Tested-by: Jeff Layton <[email protected]>
> On Oct 9, 2023, at 7:35 PM, NeilBrown <[email protected]> wrote:
> We are required to hold a reference to the svc_serv struct while
> stopping the last thread, as doing that could otherwise drop the last
> reference itself and the svc_serv would be freed while still in use.
>
> lockd doesn't do this. After startup, the only reference is held by the
> running thread.
>
> So change locked to hold a reference on nlmsvc_serv while-ever the
> service is active, and only drop it after the last thread has been
> stopped.
>
> Note: it doesn't really make sense for threads to hold references to the
> svc_serv any more. The fact threads are included in serv->sv_nrthreads
> is sufficient. Maybe a future patch could address this.
>
> Reported-by: Jeff Layton <[email protected]>
> Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
> Signed-off-by: NeilBrown <[email protected]>
Thanks for the fast response!
Should I squash this into 68cc, or apply it before? I would
like to ensure that bisect works nicely over this series of
commits.
--
Chuck Lever
On Tue, 10 Oct 2023, Chuck Lever III wrote:
>
> > On Oct 9, 2023, at 7:35 PM, NeilBrown <[email protected]> wrote:
> > We are required to hold a reference to the svc_serv struct while
> > stopping the last thread, as doing that could otherwise drop the last
> > reference itself and the svc_serv would be freed while still in use.
> >
> > lockd doesn't do this. After startup, the only reference is held by the
> > running thread.
> >
> > So change locked to hold a reference on nlmsvc_serv while-ever the
> > service is active, and only drop it after the last thread has been
> > stopped.
> >
> > Note: it doesn't really make sense for threads to hold references to the
> > svc_serv any more. The fact threads are included in serv->sv_nrthreads
> > is sufficient. Maybe a future patch could address this.
> >
> > Reported-by: Jeff Layton <[email protected]>
> > Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
> > Signed-off-by: NeilBrown <[email protected]>
>
> Thanks for the fast response!
>
> Should I squash this into 68cc, or apply it before? I would
> like to ensure that bisect works nicely over this series of
> commits.
Probably makes sense to put it before. In that case the patch
description needs re-wording.
And on reflection I think the code should be changed a little so that it
matches similar code in nfsd and nfs4-callback.
So I'll repost.
I'll take the liberty of preserving Jeff's review/test even though I've
changed the code .... I hope that's OK.
NeilBrown
> On Oct 10, 2023, at 6:30 PM, NeilBrown <[email protected]> wrote:
>
> On Tue, 10 Oct 2023, Chuck Lever III wrote:
>>
>>> On Oct 9, 2023, at 7:35 PM, NeilBrown <[email protected]> wrote:
>>> We are required to hold a reference to the svc_serv struct while
>>> stopping the last thread, as doing that could otherwise drop the last
>>> reference itself and the svc_serv would be freed while still in use.
>>>
>>> lockd doesn't do this. After startup, the only reference is held by the
>>> running thread.
>>>
>>> So change locked to hold a reference on nlmsvc_serv while-ever the
>>> service is active, and only drop it after the last thread has been
>>> stopped.
>>>
>>> Note: it doesn't really make sense for threads to hold references to the
>>> svc_serv any more. The fact threads are included in serv->sv_nrthreads
>>> is sufficient. Maybe a future patch could address this.
>>>
>>> Reported-by: Jeff Layton <[email protected]>
>>> Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
>>> Signed-off-by: NeilBrown <[email protected]>
>>
>> Thanks for the fast response!
>>
>> Should I squash this into 68cc, or apply it before? I would
>> like to ensure that bisect works nicely over this series of
>> commits.
>
> Probably makes sense to put it before. In that case the patch
> description needs re-wording.
>
> And on reflection I think the code should be changed a little so that it
> matches similar code in nfsd and nfs4-callback.
> So I'll repost.
> I'll take the liberty of preserving Jeff's review/test even though I've
> changed the code .... I hope that's OK.
Sounds like a plan. I've picked up v2 and applied it to nfsd-next.
--
Chuck Lever