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

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-05-01 at 08:29 -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    | 12 ++++++------
>  include/linux/nfs4.h |  1 +
>  4 files changed, 26 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 6e95db6c17e9..c969e6b0dd84 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;
>  
> @@ -2904,6 +2902,7 @@ static int nfs41_test_and_free_expired_stateid(struct nfs_server *server,
>  		break;
>  	case NFS4_INVALID_STATEID_TYPE:
>  	case NFS4_SPECIAL_STATEID_TYPE:
> +	case NFS4_FREED_STATEID_TYPE:
>  		return -NFS4ERR_BAD_STATEID;
>  	case NFS4_REVOKED_STATEID_TYPE:
>  		goto out_free;
> @@ -10570,7 +10569,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)
>  {
> @@ -10610,6 +10609,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;
>  };
>  

Seems like a good idea to me!

Reviewed-by: Jeff Layton <jlayton@kernel.org>