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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
