| Message ID | 169689454310.26263.15848180396022999880@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lockd: hold a reference to nlmsvc_serv while stopping thread. | expand |
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 <jlayton@kernel.org> > Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.") > Signed-off-by: NeilBrown <neilb@suse.de> > --- > > 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 <jlayton@kernel.org> Tested-by: Jeff Layton <jlayton@kernel.org>
> On Oct 9, 2023, at 7:35 PM, NeilBrown <neilb@suse.de> 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 <jlayton@kernel.org> > Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.") > Signed-off-by: NeilBrown <neilb@suse.de> 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 <neilb@suse.de> 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 <jlayton@kernel.org> > > Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.") > > Signed-off-by: NeilBrown <neilb@suse.de> > > 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 <neilb@suse.de> wrote: > > On Tue, 10 Oct 2023, Chuck Lever III wrote: >> >>> On Oct 9, 2023, at 7:35 PM, NeilBrown <neilb@suse.de> 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 <jlayton@kernel.org> >>> Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.") >>> Signed-off-by: NeilBrown <neilb@suse.de> >> >> 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
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"); }
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 <jlayton@kernel.org> Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.") Signed-off-by: NeilBrown <neilb@suse.de> --- Thanks for the report Jeff !!! fs/lockd/svc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)