On Sat, 2025-02-08 at 14:18 -0500, Tom Talpey wrote:
> On 2/8/2025 11:08 AM, Jeff Layton wrote:
> > On Sat, 2025-02-08 at 13:40 -0500, Tom Talpey wrote:
> > > On 2/8/2025 10:02 AM, Jeff Layton wrote:
> > > > On Sat, 2025-02-08 at 12:01 -0500, Chuck Lever wrote:
> > > > > On 2/7/25 4:53 PM, Jeff Layton wrote:
> > > > > > For NFS4ERR_SEQ_MISORDERED, do one attempt with a seqid of 1, and then
> > > > > > fall back to treating it like a BADSLOT if that fails.
> > > > > >
> > > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > > > ---
> > > > > > fs/nfsd/nfs4callback.c | 16 ++++++++++------
> > > > > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > > > > > index 10067a34db3afff8d4e4383854ab9abd9767c2d6..d6e3e8bb2efabadda9f922318880e12e1cb2c23f 100644
> > > > > > --- a/fs/nfsd/nfs4callback.c
> > > > > > +++ b/fs/nfsd/nfs4callback.c
> > > > > > @@ -1393,6 +1393,16 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
> > > > > > goto requeue;
> > > > > > rpc_delay(task, 2 * HZ);
> > > > > > return false;
> > > > > > + case -NFS4ERR_SEQ_MISORDERED:
> > > > > > + /*
> > > > > > + * Reattempt once with seq_nr 1. If that fails, treat this
> > > > > > + * like BADSLOT.
> > > > > > + */
> > > > >
> > > > > Nit: this comment says exactly what the code says. If it were me, I'd
> > > > > remove it. Is there a "why" statement that could be made here? Like,
> > > > > why retry with a seq_nr of 1 instead of just failing immediately?
> > > > >
> > > >
> > > > There isn't one that I know of. It looks like Kinglong Mee added it in
> > > > 7ba6cad6c88f, but there is no real mention of that in the changelog.
> > > >
> > > > TBH, I'm not enamored with this remedy either. What if the seq_nr was 2
> > > > when we got this error, and we then retry with a seq_nr of 1? Does the
> > > > server then treat that as a retransmission?
> > >
> > > So I assume you mean the requester sent seq_nr 1, saw a reply and sent a
> > > subsequent seq_nr 2, to which it gets SEQ_MISORDERED.
> > >
> > > If so, yes definitely backing up the seq_nr to 1 will result in the
> > > peer considering it to be a retransmission, which will be bad.
> > >
> >
> > Yes, that's what I meant.
> >
> > > > We might be best off
> > > > dropping this and just always treating it like BADSLOT.
> > >
> > > But, why would this happen? Usually I'd think the peer sent seq_nr X
> > > before it received a reply to seq_nr X-1, which would be a peer bug.
> > >
> > > OTOH, SEQ_MISORDERED is a valid response to an in-progress retry. So,
> > > how does the requester know the difference?
> > >
> > > If treating it as BADSLOT completely resets the sequence, then sure,
> > > but either a) the request is still in-progress, or b) if a bug is
> > > causing the situation, well it's not going to converge on a functional
> > > session.
> > >
> >
> > With this patchset, on BADSLOT, we'll set SEQ4_STATUS_BACKCHANNEL_FAULT
> > in the next forechannel SEQUENCE on the session. That should cause the
> > client to (eventually) send a DESTROY_SESSION and create a new one.
> >
> > Unfortunately, in the meantime, because of the way the callback channel
> > update works, the server can end up trying to send the callback again
> > on the same session (and maybe more than once). I'm not sure that
> > that's a real problem per-se, but it's less than ideal.
> >
> > > Not sure I have a solid suggestion right now. Whatever the fix, it
> > > should capture any subtlety in a comment.
> > >
> >
> > At this point, I'm leaning toward just treating it like BADSLOT.
> > Basically, mark the backchannel faulty, and leak the slot so that
> > nothing else uses it. That allows us to send backchannel requests on
> > the other slots until the session gets recreated.
>
> Hmm, leaking the slot is a workable approach, as long as it doesn't
> cascade more than a time or two. Some sort of trigger should be armed
> to prevent runaway retries.
>
> It's maybe worth considering what state the peer might be in when this
> happens. It too may effectively leak a slot, and if is retaining some
> bogus state either as a result of or because of the previous exchange(s)
> then this may lead to future hangs/failures. Not pretty, and maybe not
> worth trying to guess.
>
> Tom.
>
The idea here is that eventually the client should figure out that
something is wrong and reestablish the session. Currently we don't
limit the number of retries on a callback.
Maybe they should time out after a while? If we've retried a callback
for more than two lease periods, give up and log something?
Either way, I'd consider that to be follow-on work to this set.
> >
> > > >
> > > > Thoughts?
> > > >
> > > > >
> > > > > > + if (session->se_cb_seq_nr[cb->cb_held_slot] != 1) {
> > > > > > + session->se_cb_seq_nr[cb->cb_held_slot] = 1;
> > > > > > + goto retry_nowait;
> > > > > > + }
> > > > > > + fallthrough;
> > > > > > case -NFS4ERR_BADSLOT:
> > > > > > /*
> > > > > > * BADSLOT means that the client and server are out of sync
> > > > > > @@ -1403,12 +1413,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
> > > > > > nfsd4_mark_cb_fault(cb->cb_clp);
> > > > > > cb->cb_held_slot = -1;
> > > > > > goto retry_nowait;
> > > > > > - case -NFS4ERR_SEQ_MISORDERED:
> > > > > > - if (session->se_cb_seq_nr[cb->cb_held_slot] != 1) {
> > > > > > - session->se_cb_seq_nr[cb->cb_held_slot] = 1;
> > > > > > - goto retry_nowait;
> > > > > > - }
> > > > > > - break;
> > > > > > default:
> > > > > > nfsd4_mark_cb_fault(cb->cb_clp);
> > > > > > }
> > > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
