[PATCH v5 2/7] nfsd: always release slot when requeueing callback

Jeff Layton posted 7 patches 1 year ago
There is a newer version of this series
[PATCH v5 2/7] nfsd: always release slot when requeueing callback
Posted by Jeff Layton 1 year ago
If the callback is going to be requeued to the workqueue, then release
the slot. The callback client and session could change and the slot may
no longer be valid after that point.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4callback.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 79abc981e6416a88d9a81497e03e12faa3ce6d0e..bb5356e8713a8840bb714859618ff88130825efd 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1411,6 +1411,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
 	rpc_restart_call_prepare(task);
 	goto out;
 requeue:
+	nfsd41_cb_release_slot(cb);
 	if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) {
 		trace_nfsd_cb_restart(clp, cb);
 		task->tk_status = 0;

-- 
2.48.1
Re: [PATCH v5 2/7] nfsd: always release slot when requeueing callback
Posted by Chuck Lever 1 year ago
On 2/7/25 4:53 PM, Jeff Layton wrote:
> If the callback is going to be requeued to the workqueue, then release
> the slot. The callback client and session could change and the slot may
> no longer be valid after that point.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4callback.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 79abc981e6416a88d9a81497e03e12faa3ce6d0e..bb5356e8713a8840bb714859618ff88130825efd 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -1411,6 +1411,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
>  	rpc_restart_call_prepare(task);
>  	goto out;
>  requeue:
> +	nfsd41_cb_release_slot(cb);
>  	if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) {
>  		trace_nfsd_cb_restart(clp, cb);
>  		task->tk_status = 0;
> 

The NFSv4.0 case also goes to the requeue label. Is
nfsd41_cb_release_slot() a no-op for NFSv4.0? Even if it is, this is a
little confusing. Later, in 7/7, the nfsd41_cb_release_slot() call is
carried into the helper that is called by both NFSv4.0 and NFSv4.1, and
that doesn't seem necessary.

Perhaps this change should be done /after/ the NFSv4.0 error flow is
hoisted into nfsd4_cb_done().

-- 
Chuck Lever
Re: [PATCH v5 2/7] nfsd: always release slot when requeueing callback
Posted by Jeff Layton 1 year ago
On Sat, 2025-02-08 at 11:57 -0500, Chuck Lever wrote:
> On 2/7/25 4:53 PM, Jeff Layton wrote:
> > If the callback is going to be requeued to the workqueue, then release
> > the slot. The callback client and session could change and the slot may
> > no longer be valid after that point.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs4callback.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 79abc981e6416a88d9a81497e03e12faa3ce6d0e..bb5356e8713a8840bb714859618ff88130825efd 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -1411,6 +1411,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
> >  	rpc_restart_call_prepare(task);
> >  	goto out;
> >  requeue:
> > +	nfsd41_cb_release_slot(cb);
> >  	if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) {
> >  		trace_nfsd_cb_restart(clp, cb);
> >  		task->tk_status = 0;
> > 
> 
> The NFSv4.0 case also goes to the requeue label. Is
> nfsd41_cb_release_slot() a no-op for NFSv4.0?
> 

Yes.

> Even if it is, this is a
> little confusing. Later, in 7/7, the nfsd41_cb_release_slot() call is
> carried into the helper that is called by both NFSv4.0 and NFSv4.1, and
> that doesn't seem necessary.
> 
> Perhaps this change should be done /after/ the NFSv4.0 error flow is
> hoisted into nfsd4_cb_done().
> 

Fair point. I'll move the v4.0 rework ahead of this patch and only call
nfsd41_cb_release_slot for v4.1+. I'll also fix the changelog of #3
like you suggested.
-- 
Jeff Layton <jlayton@kernel.org>