Re: [PATCH] NFSv4: ensure the open stateid seqid doesn't go backwards

[Scott Mayhew <smayhew@redhat.com>]

On Thu, 30 Oct 2025, Benjamin J Coddington wrote:

> On 29 Oct 2025, at 15:31, Scott Mayhew wrote:
> 
> > We have observed an NFSv4 client receiving a LOCK reply with a status of
> > NFS4ERR_OLD_STATEID and subsequently retrying the LOCK request with an
> > earlier seqid value in the stateid.  As this was for a new lockowner,
> > that would imply that nfs_set_open_stateid_locked() had updated the open
> > stateid seqid with an earlier value.
> >
> > Looking at nfs_set_open_stateid_locked(), if the incoming seqid is out
> > of sequence, the task will sleep on the state->waitq for up to 5
> > seconds.  If the task waits for the full 5 seconds, then after finishing
> > the wait it'll update the open stateid seqid with whatever value the
> > incoming seqid has.  If there are multiple waiters in this scenario,
> > then the last one to perform said update may not be the one with the
> > highest seqid.
> >
> > Add a check to ensure that the seqid can only be incremented, and add a
> > tracepoint to indicate when old seqids are skipped.
> >
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  fs/nfs/nfs4proc.c  | 7 +++++++
> >  fs/nfs/nfs4trace.h | 1 +
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 411776718494..840ec732ade4 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -1780,6 +1780,13 @@ static void nfs_set_open_stateid_locked(struct nfs4_state *state,
> >                 if (nfs_stateid_is_sequential(state, stateid))
> >                         break;
> >
> > +               if (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
> 
> Should we unroll or modify nfs_stateid_is_sequential() which is already
> doing the match_other check here?  Maybe it could become
> nfs_stateid_is_sequential_or_.. skipped?  lost?

I think folding the check into nfs_stateid_is_sequential() would make
the code less readable.  nfs_stateid_is_sequential() returns a bool.  If
we add our check in there, then we'd need some extra info to indicate
why it's returning false.  Is it because the incoming stateid seqid is
more than 1 greater than the open stateid seqid (in which case we want
the caller to wait)?  Or is it <= to the open stateid seqid (in which
case we just want the caller to exit)?

I suppose we could change the return value to -1/0/1 or add and output
parameter or something.  Personally I just think it's clearer to have the
extra check.

-Scott
> 
> This is going to be a super-rare occurrence, but when it happens we should
> be aware that we're going to process the waiters out-of-order.
> 
> I'm trying to see any harmful side-effects of doing so, but not coming up
> with anything.  I guess we could have mis-ordered setting of
> NFS_DELEGATED_STATE, but I think we race to that anyway.
> 
> I've tried to think this over carefully - so:
> 
> Reviewed-by: Benjamin Coddington <ben.coddington@hammerspace.com>
> 
> Ben
> 
> 
> > +                   !nfs4_stateid_is_newer(stateid, &state->open_stateid)) {
> > +                       trace_nfs4_open_stateid_update_skip(state->inode,
> > +                                                           stateid, status);
> > +                       return;
> > +               }
> > +
> >                 if (status)
> >                         break;
> >                 /* Rely on seqids for serialisation with NFSv4.0 */
> > diff --git a/fs/nfs/nfs4trace.h b/fs/nfs/nfs4trace.h
> > index 9776d220cec3..6285128e631a 100644
> > --- a/fs/nfs/nfs4trace.h
> > +++ b/fs/nfs/nfs4trace.h
> > @@ -1353,6 +1353,7 @@ DEFINE_NFS4_INODE_STATEID_EVENT(nfs4_setattr);
> >  DEFINE_NFS4_INODE_STATEID_EVENT(nfs4_delegreturn);
> >  DEFINE_NFS4_INODE_STATEID_EVENT(nfs4_open_stateid_update);
> >  DEFINE_NFS4_INODE_STATEID_EVENT(nfs4_open_stateid_update_wait);
> > +DEFINE_NFS4_INODE_STATEID_EVENT(nfs4_open_stateid_update_skip);
> >  DEFINE_NFS4_INODE_STATEID_EVENT(nfs4_close_stateid_update_wait);
> >
> >  DECLARE_EVENT_CLASS(nfs4_getattr_event,
> > --
> > 2.51.0
>