| Message ID | 65d0248533fbdea2f6190faa1ee150d2d615344b.1677877233.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSv4 callback service ate my homework | expand |
> On Mar 3, 2023, at 4:08 PM, Benjamin Coddington <bcodding@redhat.com> wrote: > > Fix a race where kthread_stop() may prevent the threadfn from ever getting > called. If that happens the svc_rqst will not be cleaned up. > > Fixes: ed6473ddc704 ("NFSv4: Fix callback server shutdown") > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > net/sunrpc/svc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index 1fd3f5e57285..fea7ce8fba14 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -798,6 +798,7 @@ svc_start_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) > static int > svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) > { > + struct svc_rqst *rqstp; > struct task_struct *task; > unsigned int state = serv->sv_nrthreads-1; > > @@ -806,7 +807,10 @@ svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) > task = choose_victim(serv, pool, &state); > if (task == NULL) > break; > - kthread_stop(task); > + rqstp = kthread_data(task); > + /* Did we lose a race to svo_function threadfn? */ > + if (kthread_stop(task) == -EINTR) > + svc_exit_thread(rqstp); > nrservs++; > } while (nrservs < 0); > return 0; > -- > 2.31.1 > Seems sensible, applied. Is there a bugzilla link that should be included? -- Chuck Lever
On 7 Mar 2023, at 22:14, Chuck Lever III wrote: >> On Mar 3, 2023, at 4:08 PM, Benjamin Coddington <bcodding@redhat.com> wrote: >> >> Fix a race where kthread_stop() may prevent the threadfn from ever getting >> called. If that happens the svc_rqst will not be cleaned up. >> >> Fixes: ed6473ddc704 ("NFSv4: Fix callback server shutdown") >> Signed-off-by: Benjamin Coddington <bcodding@redhat.com> >> --- >> net/sunrpc/svc.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c >> index 1fd3f5e57285..fea7ce8fba14 100644 >> --- a/net/sunrpc/svc.c >> +++ b/net/sunrpc/svc.c >> @@ -798,6 +798,7 @@ svc_start_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) >> static int >> svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) >> { >> + struct svc_rqst *rqstp; >> struct task_struct *task; >> unsigned int state = serv->sv_nrthreads-1; >> >> @@ -806,7 +807,10 @@ svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) >> task = choose_victim(serv, pool, &state); >> if (task == NULL) >> break; >> - kthread_stop(task); >> + rqstp = kthread_data(task); >> + /* Did we lose a race to svo_function threadfn? */ >> + if (kthread_stop(task) == -EINTR) >> + svc_exit_thread(rqstp); >> nrservs++; >> } while (nrservs < 0); >> return 0; >> -- >> 2.31.1 >> > > Seems sensible, applied. Is there a bugzilla link that should be included? No, the issue was found in a private environment. Ben
On Fri, 2023-03-03 at 16:08 -0500, Benjamin Coddington wrote: > Fix a race where kthread_stop() may prevent the threadfn from ever getting > called. If that happens the svc_rqst will not be cleaned up. > > Fixes: ed6473ddc704 ("NFSv4: Fix callback server shutdown") > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > net/sunrpc/svc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index 1fd3f5e57285..fea7ce8fba14 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -798,6 +798,7 @@ svc_start_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) > static int > svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) > { > + struct svc_rqst *rqstp; > struct task_struct *task; > unsigned int state = serv->sv_nrthreads-1; > > @@ -806,7 +807,10 @@ svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) > task = choose_victim(serv, pool, &state); > if (task == NULL) > break; > - kthread_stop(task); > + rqstp = kthread_data(task); > + /* Did we lose a race to svo_function threadfn? */ > + if (kthread_stop(task) == -EINTR) > + svc_exit_thread(rqstp); > nrservs++; > } while (nrservs < 0); > return 0; Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index 1fd3f5e57285..fea7ce8fba14 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -798,6 +798,7 @@ svc_start_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) static int svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) { + struct svc_rqst *rqstp; struct task_struct *task; unsigned int state = serv->sv_nrthreads-1; @@ -806,7 +807,10 @@ svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) task = choose_victim(serv, pool, &state); if (task == NULL) break; - kthread_stop(task); + rqstp = kthread_data(task); + /* Did we lose a race to svo_function threadfn? */ + if (kthread_stop(task) == -EINTR) + svc_exit_thread(rqstp); nrservs++; } while (nrservs < 0); return 0;
Fix a race where kthread_stop() may prevent the threadfn from ever getting called. If that happens the svc_rqst will not be cleaned up. Fixes: ed6473ddc704 ("NFSv4: Fix callback server shutdown") Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- net/sunrpc/svc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)