Re: [RESEND PATCH v2] NFSv4: fairly test all delegations on a SEQ4_ revocation

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2023-10-19 at 11:59 -0400, Benjamin Coddington wrote:
> When the client is required to use TEST_STATEID to discover which
> delegation(s) have been revoked, it may continually test delegations at the
> head of the list if the server continues to be unsatisfied and send
> SEQ4_STATUS_RECALLABLE_STATE_REVOKED.  For a large number of delegations
> this behavior is prone to live-lock because the client may never be able to
> test and free revoked state at the end of the list since the
> SEQ4_STATUS_RECALLABLE_STATE_REVOKED will cause us to flag delegations at
> the head of the list to be tested.  This problem is further exacerbated by
> the state manager's willingness to be scheduled out on a busy system while
> testing the list of delegations.
> 
> Keep a generation counter for each attempt to test all delegations, and
> skip delegations that have already been tested in the current pass.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/delegation.c       | 7 ++++++-
>  fs/nfs/delegation.h       | 1 +
>  include/linux/nfs_fs_sb.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> --
> 
> Changed on v2: remove extra brackets that had my debug statements
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index cf7365581031..fa1a14def45c 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -448,6 +448,7 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
>  	delegation->cred = get_cred(cred);
>  	delegation->inode = inode;
>  	delegation->flags = 1<<NFS_DELEGATION_REFERENCED;
> +	delegation->test_gen = 0;
>  	spin_lock_init(&delegation->lock);
>  
>  	spin_lock(&clp->cl_lock);
> @@ -1294,6 +1295,8 @@ static int nfs_server_reap_expired_delegations(struct nfs_server *server,
>  	struct inode *inode;
>  	const struct cred *cred;
>  	nfs4_stateid stateid;
> +	unsigned long gen = ++server->delegation_gen;
> +
>  restart:
>  	rcu_read_lock();
>  restart_locked:
> @@ -1303,7 +1306,8 @@ static int nfs_server_reap_expired_delegations(struct nfs_server *server,
>  		    test_bit(NFS_DELEGATION_RETURNING,
>  					&delegation->flags) ||
>  		    test_bit(NFS_DELEGATION_TEST_EXPIRED,
> -					&delegation->flags) == 0)
> +					&delegation->flags) == 0 ||
> +			delegation->test_gen == gen)
>  			continue;
>  		inode = nfs_delegation_grab_inode(delegation);
>  		if (inode == NULL)
> @@ -1312,6 +1316,7 @@ static int nfs_server_reap_expired_delegations(struct nfs_server *server,
>  		cred = get_cred_rcu(delegation->cred);
>  		nfs4_stateid_copy(&stateid, &delegation->stateid);
>  		spin_unlock(&delegation->lock);
> +		delegation->test_gen = gen;
>  		clear_bit(NFS_DELEGATION_TEST_EXPIRED, &delegation->flags);
>  		rcu_read_unlock();
>  		nfs_delegation_test_free_expired(inode, &stateid, cred);
> diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
> index 1c378992b7c0..a6f495d012cf 100644
> --- a/fs/nfs/delegation.h
> +++ b/fs/nfs/delegation.h
> @@ -21,6 +21,7 @@ struct nfs_delegation {
>  	fmode_t type;
>  	unsigned long pagemod_limit;
>  	__u64 change_attr;
> +	unsigned long test_gen;
>  	unsigned long flags;
>  	refcount_t refcount;
>  	spinlock_t lock;
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index 20eeba8b009d..2f9d380b3439 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -238,6 +238,7 @@ struct nfs_server {
>  	struct list_head	delegations;
>  	struct list_head	ss_copies;
>  
> +	unsigned long		delegation_gen;
>  	unsigned long		mig_gen;
>  	unsigned long		mig_status;
>  #define NFS_MIG_IN_TRANSITION		(1)

Looks like a reasonable thing to do.

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