Re: [PATCH 3/8] nfsd: when CB_SEQUENCE gets NFS4ERR_DELAY, release the slot

Olga Kornievskaia
2025-01-24T17:45:38Z

On Fri, Jan 24, 2025 at 9:08 AM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, Jan 23, 2025 at 06:20:08PM -0500, Jeff Layton wrote:
> > On Thu, 2025-01-23 at 17:18 -0500, Chuck Lever wrote:
> > > On 1/23/25 3:25 PM, Jeff Layton wrote:
> > > > RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY:
> > > >
> > > > "For any of a number of reasons, the replier could not process this
> > > > operation in what was deemed a reasonable time. The client should wait
> > > > and then try the request with a new slot and sequence value."
> > >
> > > A little farther down, Section 15.1.1.3 says this:
> > >
> > > "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is
> > > retried in full with the SEQUENCE operation containing the same slot
> > > and sequence values."
> > >
> > > And:
> > >
> > > "If NFS4ERR_DELAY is returned on an operation other than the first in
> > > the request, the request when retried MUST contain a SEQUENCE operation
> > > that is different than the original one, with either the slot ID or the
> > > sequence value different from that in the original request."
> > >
> > > My impression is that the slot needs to be held and used again only if
> > > the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If
> > > the NFS4ERR_DELAY was the status of the 2nd or later operation in the
> > > COMPOUND, then yes, a different slot, or the same slot with a bumped
> > > sequence number, must be used.
> > >
> > > The current code in nfsd4_cb_sequence_done() appears to be correct in
> > > this regard.
> > >
> >
> > Ok! I stand corrected. We should be able to just drop this patch, but
> > some of the later patches may need some trivial merge conflicts fixed
> > up.
> >
> > Any idea why SEQUENCE is different in this regard?
>
> Isn't DELAY on SEQUENCE an indication that the operation is still in
> progress? The difference between retrying the same slot or not is
> whether you're asking the server again for the result of the previous
> operation, or whether you're asking it to perform a new one.
>
> If you get DELAY on a later op and then keep retrying with the same
> seqid/slot then I'd expect you to get stuck in an infinite loop getting
> a DELAY response out of the reply cache.

If the client would keep re-using the same seqid for the operation it
got a DELAY on then it would be a broken client. When the linux client
gets ERR_DELAY, it retries the operation but it increments the seqid.

>
> --b.
>
>
> > This rule seems a
> > bit arbitrary. If the response is NFS4ERR_DELAY, then why would it
> > matter which slot you use when retransmitting? The responder is just
> > saying "go away and come back later".
> >
> > What if the responder repeatedly returns NFS4ERR_DELAY (maybe because
> > it's under resource pressure), and also shrinks the slot table in the
> > meantime? It seems like that might put the requestor in an untenable
> > position.
> >
> > Maybe we should lobby to get this changed in the spec?
> >
> > >
> > > > This is CB_SEQUENCE, but I believe the same rule applies. Release the
> > > > slot before submitting the delayed RPC.
> > > >
> > > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors")
> > > > 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 bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644
> > > > --- a/fs/nfsd/nfs4callback.c
> > > > +++ b/fs/nfsd/nfs4callback.c
> > > > @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
> > > > goto need_restart;
> > > > case -NFS4ERR_DELAY:
> > > > cb->cb_seq_status = 1;
> > > > + nfsd41_cb_release_slot(cb);
> > > > if (!rpc_restart_call(task))
> > > > goto out;
> > > >
> > > >
> > >
> > >
> >
> > --
> > Jeff Layton <jlayton@kernel.org>
>