| Message ID | 20250207-nfsd-6-14-v5-2-f3b54fb60dc0@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Chuck Lever |
| Headers | show |
| Series | nfsd: CB_SEQUENCE error handling fixes and cleanups | expand |
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().
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.
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;
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(+)