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

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

[Scott Mayhew]

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) &&
+		    !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

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

[Benjamin J Coddington]

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?

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

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

[Scott Mayhew]

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
> 

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

[Benjamin Coddington]

On 31 Oct 2025, at 11:17, Scott Mayhew wrote:

> [You don't often get email from smayhew@redhat.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> 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.

I agree it makes the calling convention difficult, but it could be unrolled.
Probably the compiler will optimize away the double memcmp().

We had a good run not worrying about this case and only needing
nfs_stateid_is_sequential().

Ben