[PATCH 0/2] Allow FREE_STATEID to free delegations

[PATCH 0/2] Allow FREE_STATEID to free delegations

[Benjamin Coddington]

A problem observed for some clients is that the list of nfs_server->delegations can
grow unweildy, leading to the clients spinning in tight loops walking
across delegations that have been marked revoked.  These two patches attempt
to solve that problem by using the result of FREE_STATEID to clean up the
list of delegations, thus keeping that list pruned to an operable size.

There's a couple things not to like here: I don't like dropping the const
qualifier on the stateid for both the test and free operations.  The first
patch finishes the mostly complete work of ensuring we're passing a copy.
The core issue is that callers can't determine if FREE_STATEID was
successful since the operation is folded into test_and_free_stateid().
Another way would be to un-fold the call paths that are combined using
test_and_free friends, and that seems worse than just carrying result on the
stateid's type.

The second thing I don't like with this approach is that it doesn't fix the
potential problem that the client should try to make repeated attempts to free a
delegation stateid on the server that received an error from FREE_STATEID on the
first pass.  That's probably a pretty rare thing, but its also something
that can keep revoked state around forever potentially creating a
never-ending operation loop between client and server.

First pass, please criticise, thanks for any comments.

Ben

Benjamin Coddington (2):
  NFSv4: Ensure test_and_free_stateid callers use private memory
  NFSv4: Allow FREE_STATEID to clean up delegations

 fs/nfs/delegation.c  | 25 ++++++++++++++++++-------
 fs/nfs/nfs4_fs.h     |  3 +--
 fs/nfs/nfs4proc.c    | 23 ++++++++++++-----------
 include/linux/nfs4.h |  1 +
 4 files changed, 32 insertions(+), 20 deletions(-)

-- 
2.47.0

[PATCH 1/2] NFSv4: Ensure test_and_free_stateid callers use private memory

[Benjamin Coddington]

A follow-up patch intends to signal the success or failure of FREE_STATEID
by modifying the nfs4_stateid's type which requires the const qualifier for
the nfs4_stateid to be dropped.  Since it will no longer safe to operate
directly on shared stateid objects in this path, ensure that callers send a
copy.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/nfs4proc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 6e95db6c17e9..bfb9e980d662 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2990,6 +2990,7 @@ static void nfs41_delegation_recover_stateid(struct nfs4_state *state)
 static int nfs41_check_expired_locks(struct nfs4_state *state)
 {
 	int status, ret = NFS_OK;
+	nfs4_stateid stateid;
 	struct nfs4_lock_state *lsp, *prev = NULL;
 	struct nfs_server *server = NFS_SERVER(state->inode);
 
@@ -3007,9 +3008,9 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
 			nfs4_put_lock_state(prev);
 			prev = lsp;
 
+			nfs4_stateid_copy(&stateid, &lsp->ls_stateid);
 			status = nfs41_test_and_free_expired_stateid(server,
-					&lsp->ls_stateid,
-					cred);
+					&stateid, cred);
 			trace_nfs4_test_lock_stateid(state, lsp, status);
 			if (status == -NFS4ERR_EXPIRED ||
 			    status == -NFS4ERR_BAD_STATEID) {
@@ -3042,17 +3043,18 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
 static int nfs41_check_open_stateid(struct nfs4_state *state)
 {
 	struct nfs_server *server = NFS_SERVER(state->inode);
-	nfs4_stateid *stateid = &state->open_stateid;
+	nfs4_stateid stateid;
 	const struct cred *cred = state->owner->so_cred;
 	int status;
 
 	if (test_bit(NFS_OPEN_STATE, &state->flags) == 0)
 		return -NFS4ERR_BAD_STATEID;
-	status = nfs41_test_and_free_expired_stateid(server, stateid, cred);
+	nfs4_stateid_copy(&stateid, &state->open_stateid);
+	status = nfs41_test_and_free_expired_stateid(server, &stateid, cred);
 	trace_nfs4_test_open_stateid(state, NULL, status);
 	if (status == -NFS4ERR_EXPIRED || status == -NFS4ERR_BAD_STATEID) {
 		nfs_state_clear_open_state_flags(state);
-		stateid->type = NFS4_INVALID_STATEID_TYPE;
+		state->open_stateid.type = NFS4_INVALID_STATEID_TYPE;
 		return status;
 	}
 	if (nfs_open_stateid_recover_openmode(state))
-- 
2.47.0

[PATCH 2/2] NFSv4: Allow FREE_STATEID to clean up delegations

[Benjamin Coddington]

The NFS client's list of delegations can grow quite large (well beyond the
delegation watermark) if the server is revoking or there are repeated
events that expire state.  Once this happens, the revoked delegations can
cause a performance problem for subsequent walks of the
servers->delegations list when the client tries to test and free state.

If we can determine that the FREE_STATEID operation has completed without
error, we can prune the delegation from the list.

Since the NFS client combines TEST_STATEID with FREE_STATEID in its minor
version operations, there isn't an easy way to communicate success of
FREE_STATEID.  Rather than re-arrange quite a number of calling paths to
break out the separate procedures, let's signal the success of FREE_STATEID
by setting the stateid's type.

Set NFS4_FREED_STATEID_TYPE for stateids that have been successfully
discarded from the server, and use that type to signal that the delegation
can be cleaned up.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/delegation.c  | 25 ++++++++++++++++++-------
 fs/nfs/nfs4_fs.h     |  3 +--
 fs/nfs/nfs4proc.c    | 11 +++++------
 include/linux/nfs4.h |  1 +
 4 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 4db912f56230..b746793cf730 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -1006,13 +1006,6 @@ static void nfs_revoke_delegation(struct inode *inode,
 		nfs_inode_find_state_and_recover(inode, stateid);
 }
 
-void nfs_remove_bad_delegation(struct inode *inode,
-		const nfs4_stateid *stateid)
-{
-	nfs_revoke_delegation(inode, stateid);
-}
-EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
-
 void nfs_delegation_mark_returned(struct inode *inode,
 		const nfs4_stateid *stateid)
 {
@@ -1054,6 +1047,24 @@ void nfs_delegation_mark_returned(struct inode *inode,
 	nfs_inode_find_state_and_recover(inode, stateid);
 }
 
+/**
+ * nfs_remove_bad_delegation - handle delegations that are unusable
+ * @inode: inode to process
+ * @stateid: the delegation's stateid
+ *
+ * If the server ACK-ed our FREE_STATEID then clean
+ * up the delegation, else mark and keep the revoked state.
+ */
+void nfs_remove_bad_delegation(struct inode *inode,
+		const nfs4_stateid *stateid)
+{
+	if (stateid && stateid->type == NFS4_FREED_STATEID_TYPE)
+		nfs_delegation_mark_returned(inode, stateid);
+	else
+		nfs_revoke_delegation(inode, stateid);
+}
+EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
+
 /**
  * nfs_expire_unused_delegation_types
  * @clp: client to process
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 7d383d29a995..d3ca91f60fc1 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -67,8 +67,7 @@ struct nfs4_minor_version_ops {
 	void	(*free_lock_state)(struct nfs_server *,
 			struct nfs4_lock_state *);
 	int	(*test_and_free_expired)(struct nfs_server *,
-					 const nfs4_stateid *,
-					 const struct cred *);
+					 nfs4_stateid *, const struct cred *);
 	struct nfs_seqid *
 		(*alloc_seqid)(struct nfs_seqid_counter *, gfp_t);
 	void	(*session_trunk)(struct rpc_clnt *clnt,
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index bfb9e980d662..143cb191b0b8 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -105,7 +105,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp,
 		bool is_privileged);
 static int nfs41_test_stateid(struct nfs_server *, const nfs4_stateid *,
 			      const struct cred *);
-static int nfs41_free_stateid(struct nfs_server *, const nfs4_stateid *,
+static int nfs41_free_stateid(struct nfs_server *, nfs4_stateid *,
 			      const struct cred *, bool);
 #endif
 
@@ -2886,16 +2886,14 @@ static int nfs40_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *st
 }
 
 static int nfs40_test_and_free_expired_stateid(struct nfs_server *server,
-					       const nfs4_stateid *stateid,
-					       const struct cred *cred)
+					       nfs4_stateid *stateid, const struct cred *cred)
 {
 	return -NFS4ERR_BAD_STATEID;
 }
 
 #if defined(CONFIG_NFS_V4_1)
 static int nfs41_test_and_free_expired_stateid(struct nfs_server *server,
-					       const nfs4_stateid *stateid,
-					       const struct cred *cred)
+					       nfs4_stateid *stateid, const struct cred *cred)
 {
 	int status;
 
@@ -10572,7 +10570,7 @@ static const struct rpc_call_ops nfs41_free_stateid_ops = {
  * Note: this function is always asynchronous.
  */
 static int nfs41_free_stateid(struct nfs_server *server,
-		const nfs4_stateid *stateid,
+		nfs4_stateid *stateid,
 		const struct cred *cred,
 		bool privileged)
 {
@@ -10612,6 +10610,7 @@ static int nfs41_free_stateid(struct nfs_server *server,
 	if (IS_ERR(task))
 		return PTR_ERR(task);
 	rpc_put_task(task);
+	stateid->type = NFS4_FREED_STATEID_TYPE;
 	return 0;
 }
 
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index 9ac83ca88326..8ec5766cb22f 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -72,6 +72,7 @@ struct nfs4_stateid_struct {
 		NFS4_LAYOUT_STATEID_TYPE,
 		NFS4_PNFS_DS_STATEID_TYPE,
 		NFS4_REVOKED_STATEID_TYPE,
+		NFS4_FREED_STATEID_TYPE,
 	} type;
 };
 
-- 
2.47.0

Re: [PATCH 2/2] NFSv4: Allow FREE_STATEID to clean up delegations

[Jeff Layton]

On Wed, 2025-04-23 at 13:59 -0400, Benjamin Coddington wrote:
> The NFS client's list of delegations can grow quite large (well beyond the
> delegation watermark) if the server is revoking or there are repeated
> events that expire state.  Once this happens, the revoked delegations can
> cause a performance problem for subsequent walks of the
> servers->delegations list when the client tries to test and free state.
> 
> If we can determine that the FREE_STATEID operation has completed without
> error, we can prune the delegation from the list.
> 
> Since the NFS client combines TEST_STATEID with FREE_STATEID in its minor
> version operations, there isn't an easy way to communicate success of
> FREE_STATEID.  Rather than re-arrange quite a number of calling paths to
> break out the separate procedures, let's signal the success of FREE_STATEID
> by setting the stateid's type.
> 
> Set NFS4_FREED_STATEID_TYPE for stateids that have been successfully
> discarded from the server, and use that type to signal that the delegation
> can be cleaned up.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/delegation.c  | 25 ++++++++++++++++++-------
>  fs/nfs/nfs4_fs.h     |  3 +--
>  fs/nfs/nfs4proc.c    | 11 +++++------
>  include/linux/nfs4.h |  1 +
>  4 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 4db912f56230..b746793cf730 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -1006,13 +1006,6 @@ static void nfs_revoke_delegation(struct inode *inode,
>  		nfs_inode_find_state_and_recover(inode, stateid);
>  }
>  
> -void nfs_remove_bad_delegation(struct inode *inode,
> -		const nfs4_stateid *stateid)
> -{
> -	nfs_revoke_delegation(inode, stateid);
> -}
> -EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
> -
>  void nfs_delegation_mark_returned(struct inode *inode,
>  		const nfs4_stateid *stateid)
>  {
> @@ -1054,6 +1047,24 @@ void nfs_delegation_mark_returned(struct inode *inode,
>  	nfs_inode_find_state_and_recover(inode, stateid);
>  }
>  
> +/**
> + * nfs_remove_bad_delegation - handle delegations that are unusable
> + * @inode: inode to process
> + * @stateid: the delegation's stateid
> + *
> + * If the server ACK-ed our FREE_STATEID then clean
> + * up the delegation, else mark and keep the revoked state.
> + */
> +void nfs_remove_bad_delegation(struct inode *inode,
> +		const nfs4_stateid *stateid)
> +{
> +	if (stateid && stateid->type == NFS4_FREED_STATEID_TYPE)
> +		nfs_delegation_mark_returned(inode, stateid);
> +	else
> +		nfs_revoke_delegation(inode, stateid);
> +}
> +EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
> +
>  /**
>   * nfs_expire_unused_delegation_types
>   * @clp: client to process
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 7d383d29a995..d3ca91f60fc1 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -67,8 +67,7 @@ struct nfs4_minor_version_ops {
>  	void	(*free_lock_state)(struct nfs_server *,
>  			struct nfs4_lock_state *);
>  	int	(*test_and_free_expired)(struct nfs_server *,
> -					 const nfs4_stateid *,
> -					 const struct cred *);
> +					 nfs4_stateid *, const struct cred *);
>  	struct nfs_seqid *
>  		(*alloc_seqid)(struct nfs_seqid_counter *, gfp_t);
>  	void	(*session_trunk)(struct rpc_clnt *clnt,
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index bfb9e980d662..143cb191b0b8 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -105,7 +105,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp,
>  		bool is_privileged);
>  static int nfs41_test_stateid(struct nfs_server *, const nfs4_stateid *,
>  			      const struct cred *);
> -static int nfs41_free_stateid(struct nfs_server *, const nfs4_stateid *,
> +static int nfs41_free_stateid(struct nfs_server *, nfs4_stateid *,
>  			      const struct cred *, bool);
>  #endif
>  
> @@ -2886,16 +2886,14 @@ static int nfs40_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *st
>  }
>  
>  static int nfs40_test_and_free_expired_stateid(struct nfs_server *server,
> -					       const nfs4_stateid *stateid,
> -					       const struct cred *cred)
> +					       nfs4_stateid *stateid, const struct cred *cred)
>  {
>  	return -NFS4ERR_BAD_STATEID;
>  }
>  
>  #if defined(CONFIG_NFS_V4_1)
>  static int nfs41_test_and_free_expired_stateid(struct nfs_server *server,
> -					       const nfs4_stateid *stateid,
> -					       const struct cred *cred)
> +					       nfs4_stateid *stateid, const struct cred *cred)
>  {
>  	int status;
>  
> @@ -10572,7 +10570,7 @@ static const struct rpc_call_ops nfs41_free_stateid_ops = {
>   * Note: this function is always asynchronous.
>   */
>  static int nfs41_free_stateid(struct nfs_server *server,
> -		const nfs4_stateid *stateid,
> +		nfs4_stateid *stateid,
>  		const struct cred *cred,
>  		bool privileged)
>  {
> @@ -10612,6 +10610,7 @@ static int nfs41_free_stateid(struct nfs_server *server,
>  	if (IS_ERR(task))
>  		return PTR_ERR(task);
>  	rpc_put_task(task);
> +	stateid->type = NFS4_FREED_STATEID_TYPE;

Would it be possible to call nfs_delegation_mark_returned() at this
point, and skip all of the type changing?

>  	return 0;
>  }
>  
> diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> index 9ac83ca88326..8ec5766cb22f 100644
> --- a/include/linux/nfs4.h
> +++ b/include/linux/nfs4.h
> @@ -72,6 +72,7 @@ struct nfs4_stateid_struct {
>  		NFS4_LAYOUT_STATEID_TYPE,
>  		NFS4_PNFS_DS_STATEID_TYPE,
>  		NFS4_REVOKED_STATEID_TYPE,
> +		NFS4_FREED_STATEID_TYPE,
>  	} type;
>  };
>  

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] NFSv4: Allow FREE_STATEID to clean up delegations

[Benjamin Coddington]

On 23 Apr 2025, at 15:41, Jeff Layton wrote:

> On Wed, 2025-04-23 at 13:59 -0400, Benjamin Coddington wrote:
>> @@ -10612,6 +10610,7 @@ static int nfs41_free_stateid(struct nfs_server *server,
>>  	if (IS_ERR(task))
>>  		return PTR_ERR(task);
>>  	rpc_put_task(task);
>> +	stateid->type = NFS4_FREED_STATEID_TYPE;
>
> Would it be possible to call nfs_delegation_mark_returned() at this
> point, and skip all of the type changing?

It won't because we can be here with a lock stateid or open
stateid.

Ben

Re: [PATCH 1/2] NFSv4: Ensure test_and_free_stateid callers use private memory

[Jeff Layton]

On Wed, 2025-04-23 at 13:59 -0400, Benjamin Coddington wrote:
> A follow-up patch intends to signal the success or failure of FREE_STATEID
> by modifying the nfs4_stateid's type which requires the const qualifier for
> the nfs4_stateid to be dropped.  Since it will no longer safe to operate
> directly on shared stateid objects in this path, ensure that callers send a
> copy.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/nfs4proc.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 6e95db6c17e9..bfb9e980d662 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2990,6 +2990,7 @@ static void nfs41_delegation_recover_stateid(struct nfs4_state *state)
>  static int nfs41_check_expired_locks(struct nfs4_state *state)
>  {
>  	int status, ret = NFS_OK;
> +	nfs4_stateid stateid;
>  	struct nfs4_lock_state *lsp, *prev = NULL;
>  	struct nfs_server *server = NFS_SERVER(state->inode);
>  
> @@ -3007,9 +3008,9 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>  			nfs4_put_lock_state(prev);
>  			prev = lsp;
>  
> +			nfs4_stateid_copy(&stateid, &lsp->ls_stateid);
>  			status = nfs41_test_and_free_expired_stateid(server,
> -					&lsp->ls_stateid,
> -					cred);
> +					&stateid, cred);
>  			trace_nfs4_test_lock_stateid(state, lsp, status);
>  			if (status == -NFS4ERR_EXPIRED ||
>  			    status == -NFS4ERR_BAD_STATEID) {
> @@ -3042,17 +3043,18 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>  static int nfs41_check_open_stateid(struct nfs4_state *state)
>  {
>  	struct nfs_server *server = NFS_SERVER(state->inode);
> -	nfs4_stateid *stateid = &state->open_stateid;
> +	nfs4_stateid stateid;
>  	const struct cred *cred = state->owner->so_cred;
>  	int status;
>  
>  	if (test_bit(NFS_OPEN_STATE, &state->flags) == 0)
>  		return -NFS4ERR_BAD_STATEID;
> -	status = nfs41_test_and_free_expired_stateid(server, stateid, cred);
> +	nfs4_stateid_copy(&stateid, &state->open_stateid);
> +	status = nfs41_test_and_free_expired_stateid(server, &stateid, cred);
>  	trace_nfs4_test_open_stateid(state, NULL, status);
>  	if (status == -NFS4ERR_EXPIRED || status == -NFS4ERR_BAD_STATEID) {
>  		nfs_state_clear_open_state_flags(state);
> -		stateid->type = NFS4_INVALID_STATEID_TYPE;
> +		state->open_stateid.type = NFS4_INVALID_STATEID_TYPE;
>  		return status;
>  	}
>  	if (nfs_open_stateid_recover_openmode(state))

I don't know that you really need to do this. In the cases where you
end up setting the type to FREED, you will also return NFS4ERR_EXPIRED,
which will make the callers set the type to INVALID.

There will be a brief window where the type will be set to FREED, but
that should be no big deal.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] NFSv4: Allow FREE_STATEID to clean up delegations

[Jeff Layton]

On Wed, 2025-04-23 at 15:59 -0400, Benjamin Coddington wrote:
> On 23 Apr 2025, at 15:41, Jeff Layton wrote:
> 
> > On Wed, 2025-04-23 at 13:59 -0400, Benjamin Coddington wrote:
> > > @@ -10612,6 +10610,7 @@ static int nfs41_free_stateid(struct nfs_server *server,
> > >  	if (IS_ERR(task))
> > >  		return PTR_ERR(task);
> > >  	rpc_put_task(task);
> > > +	stateid->type = NFS4_FREED_STATEID_TYPE;
> > 
> > Would it be possible to call nfs_delegation_mark_returned() at this
> > point, and skip all of the type changing?
> 
> It won't because we can be here with a lock stateid or open
> stateid.
> 

Ok, I can see why you decided to do it this way, and since we already
have a REVOKED and INVALID types, adding FREED doesn't seem that bad.

If you do go this route, then I think you also need to add the FREED
case to the switch in nfs41_test_and_free_expired_stateid().

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] NFSv4: Ensure test_and_free_stateid callers use private memory

[Benjamin Coddington]

On 23 Apr 2025, at 16:35, Jeff Layton wrote:

> On Wed, 2025-04-23 at 13:59 -0400, Benjamin Coddington wrote:
>> A follow-up patch intends to signal the success or failure of FREE_STATEID
>> by modifying the nfs4_stateid's type which requires the const qualifier for
>> the nfs4_stateid to be dropped.  Since it will no longer safe to operate
>> directly on shared stateid objects in this path, ensure that callers send a
>> copy.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>  fs/nfs/nfs4proc.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 6e95db6c17e9..bfb9e980d662 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -2990,6 +2990,7 @@ static void nfs41_delegation_recover_stateid(struct nfs4_state *state)
>>  static int nfs41_check_expired_locks(struct nfs4_state *state)
>>  {
>>  	int status, ret = NFS_OK;
>> +	nfs4_stateid stateid;
>>  	struct nfs4_lock_state *lsp, *prev = NULL;
>>  	struct nfs_server *server = NFS_SERVER(state->inode);
>>
>> @@ -3007,9 +3008,9 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>>  			nfs4_put_lock_state(prev);
>>  			prev = lsp;
>>
>> +			nfs4_stateid_copy(&stateid, &lsp->ls_stateid);
>>  			status = nfs41_test_and_free_expired_stateid(server,
>> -					&lsp->ls_stateid,
>> -					cred);
>> +					&stateid, cred);
>>  			trace_nfs4_test_lock_stateid(state, lsp, status);
>>  			if (status == -NFS4ERR_EXPIRED ||
>>  			    status == -NFS4ERR_BAD_STATEID) {
>> @@ -3042,17 +3043,18 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>>  static int nfs41_check_open_stateid(struct nfs4_state *state)
>>  {
>>  	struct nfs_server *server = NFS_SERVER(state->inode);
>> -	nfs4_stateid *stateid = &state->open_stateid;
>> +	nfs4_stateid stateid;
>>  	const struct cred *cred = state->owner->so_cred;
>>  	int status;
>>
>>  	if (test_bit(NFS_OPEN_STATE, &state->flags) == 0)
>>  		return -NFS4ERR_BAD_STATEID;
>> -	status = nfs41_test_and_free_expired_stateid(server, stateid, cred);
>> +	nfs4_stateid_copy(&stateid, &state->open_stateid);
>> +	status = nfs41_test_and_free_expired_stateid(server, &stateid, cred);
>>  	trace_nfs4_test_open_stateid(state, NULL, status);
>>  	if (status == -NFS4ERR_EXPIRED || status == -NFS4ERR_BAD_STATEID) {
>>  		nfs_state_clear_open_state_flags(state);
>> -		stateid->type = NFS4_INVALID_STATEID_TYPE;
>> +		state->open_stateid.type = NFS4_INVALID_STATEID_TYPE;
>>  		return status;
>>  	}
>>  	if (nfs_open_stateid_recover_openmode(state))
>
> I don't know that you really need to do this. In the cases where you
> end up setting the type to FREED, you will also return NFS4ERR_EXPIRED,
> which will make the callers set the type to INVALID.
>
> There will be a brief window where the type will be set to FREED, but
> that should be no big deal.

Definitely playing it safe here, I think I have to worry about concurrency -
who else might be simultaneously looking at that nfs4_state->open_stateid.type?
I really don't want to introduce a regression because we knew we weren't
modifying that stateid before.

I suppose I can dive into an audit and see if that can actually happen.

Thanks for looking at this,

Ben

Re: [PATCH 2/2] NFSv4: Allow FREE_STATEID to clean up delegations

[Benjamin Coddington]

On 23 Apr 2025, at 16:37, Jeff Layton wrote:

> On Wed, 2025-04-23 at 15:59 -0400, Benjamin Coddington wrote:
>> On 23 Apr 2025, at 15:41, Jeff Layton wrote:
>>
>>> On Wed, 2025-04-23 at 13:59 -0400, Benjamin Coddington wrote:
>>>> @@ -10612,6 +10610,7 @@ static int nfs41_free_stateid(struct nfs_server *server,
>>>>  	if (IS_ERR(task))
>>>>  		return PTR_ERR(task);
>>>>  	rpc_put_task(task);
>>>> +	stateid->type = NFS4_FREED_STATEID_TYPE;
>>>
>>> Would it be possible to call nfs_delegation_mark_returned() at this
>>> point, and skip all of the type changing?
>>
>> It won't because we can be here with a lock stateid or open
>> stateid.
>>
>
> Ok, I can see why you decided to do it this way, and since we already
> have a REVOKED and INVALID types, adding FREED doesn't seem that bad.
>
> If you do go this route, then I think you also need to add the FREED
> case to the switch in nfs41_test_and_free_expired_stateid().

It's a good idea, it can't hurt, but I think its not possible to end up with
a delegation (or other type) that has a stateid with that type.  We're just
setting that type on the stack copy that we use for the
test_stateid/free_stateid operation, and then its discarded.

Ben

Re: [PATCH 1/2] NFSv4: Ensure test_and_free_stateid callers use private memory

[Benjamin Coddington]

On 23 Apr 2025, at 18:04, Benjamin Coddington wrote:

> On 23 Apr 2025, at 16:35, Jeff Layton wrote:
>
>> On Wed, 2025-04-23 at 13:59 -0400, Benjamin Coddington wrote:
>>> A follow-up patch intends to signal the success or failure of FREE_STATEID
>>> by modifying the nfs4_stateid's type which requires the const qualifier for
>>> the nfs4_stateid to be dropped.  Since it will no longer safe to operate
>>> directly on shared stateid objects in this path, ensure that callers send a
>>> copy.
>>>
>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>>> ---
>>>  fs/nfs/nfs4proc.c | 12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 6e95db6c17e9..bfb9e980d662 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -2990,6 +2990,7 @@ static void nfs41_delegation_recover_stateid(struct nfs4_state *state)
>>>  static int nfs41_check_expired_locks(struct nfs4_state *state)
>>>  {
>>>  	int status, ret = NFS_OK;
>>> +	nfs4_stateid stateid;
>>>  	struct nfs4_lock_state *lsp, *prev = NULL;
>>>  	struct nfs_server *server = NFS_SERVER(state->inode);
>>>
>>> @@ -3007,9 +3008,9 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>>>  			nfs4_put_lock_state(prev);
>>>  			prev = lsp;
>>>
>>> +			nfs4_stateid_copy(&stateid, &lsp->ls_stateid);
>>>  			status = nfs41_test_and_free_expired_stateid(server,
>>> -					&lsp->ls_stateid,
>>> -					cred);
>>> +					&stateid, cred);
>>>  			trace_nfs4_test_lock_stateid(state, lsp, status);
>>>  			if (status == -NFS4ERR_EXPIRED ||
>>>  			    status == -NFS4ERR_BAD_STATEID) {
>>> @@ -3042,17 +3043,18 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>>>  static int nfs41_check_open_stateid(struct nfs4_state *state)
>>>  {
>>>  	struct nfs_server *server = NFS_SERVER(state->inode);
>>> -	nfs4_stateid *stateid = &state->open_stateid;
>>> +	nfs4_stateid stateid;
>>>  	const struct cred *cred = state->owner->so_cred;
>>>  	int status;
>>>
>>>  	if (test_bit(NFS_OPEN_STATE, &state->flags) == 0)
>>>  		return -NFS4ERR_BAD_STATEID;
>>> -	status = nfs41_test_and_free_expired_stateid(server, stateid, cred);
>>> +	nfs4_stateid_copy(&stateid, &state->open_stateid);
>>> +	status = nfs41_test_and_free_expired_stateid(server, &stateid, cred);
>>>  	trace_nfs4_test_open_stateid(state, NULL, status);
>>>  	if (status == -NFS4ERR_EXPIRED || status == -NFS4ERR_BAD_STATEID) {
>>>  		nfs_state_clear_open_state_flags(state);
>>> -		stateid->type = NFS4_INVALID_STATEID_TYPE;
>>> +		state->open_stateid.type = NFS4_INVALID_STATEID_TYPE;
>>>  		return status;
>>>  	}
>>>  	if (nfs_open_stateid_recover_openmode(state))
>>
>> I don't know that you really need to do this. In the cases where you
>> end up setting the type to FREED, you will also return NFS4ERR_EXPIRED,
>> which will make the callers set the type to INVALID.
>>
>> There will be a brief window where the type will be set to FREED, but
>> that should be no big deal.
>
> Definitely playing it safe here, I think I have to worry about concurrency -
> who else might be simultaneously looking at that nfs4_state->open_stateid.type?
> I really don't want to introduce a regression because we knew we weren't
> modifying that stateid before.
>
> I suppose I can dive into an audit and see if that can actually happen.

After looking theoretically there shouldn't be a race in either of these two
paths since we'll only be here doing reclaim/recovery and operating on the
state sequentially.  State recovery will have drained and blocked parallel
opens.

I'll send just the 2nd patch in another version.

Ben