Message-ID: <1ece2978-239c-4939-bb16-0c7c64614c66@huawei.com>
Date: Tue, 2 Sep 2025 20:10:42 +0800
From: Li Lingfeng <lilingfeng3@...wei.com>
To: Jeff Layton <jlayton@...nel.org>, <chuck.lever@...cle.com>,
	<neil@...wn.name>, <okorniev@...hat.com>, <Dai.Ngo@...cle.com>,
	<tom@...pey.com>, <linux-nfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: <yukuai1@...weicloud.com>, <houtao1@...wei.com>, <yi.zhang@...wei.com>,
	<yangerkun@...wei.com>, <lilingfeng@...weicloud.com>,
	<zhangjian496@...wei.com>
Subject: Re: [PATCH] nfsd: remove long-standing revoked delegations by force

Hi,

在 2025/9/2 18:21, Jeff Layton 写道:
> On Tue, 2025-09-02 at 10:22 +0800, Li Lingfeng wrote:
>> When file access conflicts occur between clients, the server recalls
>> delegations. If the client holding delegation fails to return it after
>> a recall, nfs4_laundromat adds the delegation to cl_revoked list.
>> This causes subsequent SEQUENCE operations to set the
>> SEQ4_STATUS_RECALLABLE_STATE_REVOKED flag, forcing the client to
>> validate all delegations and return the revoked one.
>>
>> However, if the client fails to return the delegation due to a timeout
>> after receiving the recall or a server bug, the delegation remains in the
>> server's cl_revoked list. The client marks it revoked and won't find it
>> upon detecting SEQ4_STATUS_RECALLABLE_STATE_REVOKED. This leads to a loop:
>> the server persistently sets SEQ4_STATUS_RECALLABLE_STATE_REVOKED, and the
>> client repeatedly tests all delegations, severely impacting performance
>> when numerous delegations exist.
>>
> It is a performance impact, but I don't get the "loop" here. Are you
> saying that this problem compounds itself? That testing all delegations
> causes others to be revoked?
The delegation will be removed from server->delegations in client after
NFSPROC4_CLNT_DELEGRETURN is performed.
nfs4_delegreturn_done
  nfs_delegation_mark_returned
   nfs_detach_delegation
    nfs_detach_delegation_locked
     list_del_rcu // remove delegation from server->delegations

 From the client's perspective, the delegation has been returned, but on
the server side, it is left in the cl_revoked list.[1].

Subsequently, every sequence from the client will be flagged with
SEQ4_STATUS_RECALLABLE_STATE_REVOKED as long as cl_revoked remains 
non-empty.
nfsd4_sequence
  seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED

When the client detects SEQ4_STATUS_RECALLABLE_STATE_REVOKED while
processing a sequence result, it sets NFS_DELEGATION_TEST_EXPIRED for all
delegations and wakes up the state manager for handling.
nfs41_sequence_done
  nfs41_sequence_process
   nfs41_handle_sequence_flag_errors
    nfs41_handle_recallable_state_revoked
     nfs_test_expired_all_delegations
      nfs_mark_test_expired_all_delegations
       nfs_delegation_mark_test_expired_server
        // set NFS_DELEGATION_TEST_EXPIRED for delegations in 
server->delegations
      nfs4_schedule_state_manager

The state manager tests all delegations except the one that was returned,
as it is no longer in server->delegations.
nfs4_state_manager
  nfs4_begin_drain_session
  nfs_reap_expired_delegations
   nfs_server_reap_expired_delegations
    // test delegations in server->delegations

There may be a loop:
1) send a sequence(client)
2) return SEQ4_STATUS_RECALLABLE_STATE_REVOKED(server)
3) set NFS_DELEGATION_TEST_EXPIRED for all delegations(client)
4) test all delegations by state manager(client)
5) send another sequence(client)

The state manager's traversal of delegations occurs between
nfs4_begin_drain_session and nfs4_end_drain_session. Non-privileged requests
will be blocked because the NFS4_SLOT_TBL_DRAINING flag is set. If there are
many delegations to traverse, this blocking time can be relatively long.
>> Since abnormal delegations are removed from flc_lease via nfs4_laundromat
>> --> revoke_delegation --> destroy_unhashed_deleg -->
>> nfs4_unlock_deleg_lease --> kernel_setlease, and do not block new open
>> requests indefinitely, retaining such a delegation on the server is
>> unnecessary.
>>
>> Reported-by: Zhang Jian <zhangjian496@...wei.com>
>> Fixes: 3bd64a5ba171 ("nfsd4: implement SEQ4_STATUS_RECALLABLE_STATE_REVOKED")
>> Closes: https://lore.kernel.org/all/ff8debe9-6877-4cf7-ba29-fc98eae0ffa0@huawei.com/
>> Signed-off-by: Li Lingfeng <lilingfeng3@...wei.com>
>> ---
>>   fs/nfsd/nfs4state.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 88c347957da5..aa65a685dbb9 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4326,6 +4326,8 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>   	int buflen;
>>   	struct net *net = SVC_NET(rqstp);
>>   	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>> +	struct list_head *pos, *next;
>> +	struct nfs4_delegation *dp;
>>   
>>   	if (resp->opcnt != 1)
>>   		return nfserr_sequence_pos;
>> @@ -4470,6 +4472,15 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>   	default:
>>   		seq->status_flags = 0;
>>   	}
>> +	if (!list_empty(&clp->cl_revoked)) {
>> +		list_for_each_safe(pos, next, &clp->cl_revoked) {
>> +			dp = list_entry(pos, struct nfs4_delegation, dl_recall_lru);
>> +			if (dp->dl_time < (ktime_get_boottime_seconds() - 2 * nn->nfsd4_lease)) {
>> +				list_del_init(&dp->dl_recall_lru);
>> +				nfs4_put_stid(&dp->dl_stid);
>> +			}
>> +		}
>> +	}
>>   	if (!list_empty(&clp->cl_revoked))
>>   		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
>>   	if (atomic_read(&clp->cl_admin_revoked))
> This seems like a violation of the spec. AIUI, the server is required
> to hang onto a record of the delegation until the client does the
> TEST_STATEID/FREE_STATEID dance to remove it. Just discarding them like
> this seems wrong.
Our expected outcome was that the client would release the abnormal
delegation via TEST_STATEID/FREE_STATEID upon detecting its invalidity.
However, this problematic delegation is no longer present in the
client's server->delegations list—whether due to client-side timeouts or
the server-side bug [1].
>
> Should we instead just administratively evict the client since it's
> clearly not behaving right in this case?
Thanks for the suggestion. While administratively evicting the client would
certainly resolve the immediate delegation issue, I'm concerned that 
approach
might be a bit heavy-handed.
The problematic behavior seems isolated to a single delegation. Meanwhile,
the client itself likely has numerous other open files and active state on
the server. Forcing a complete client reconnect would tear down all that
state, which could cause significant application disruption and be perceived
as a service outage from the client's perspective.

[1] 
https://lore.kernel.org/all/de669327-c93a-49e5-a53b-bda9e67d34a2@huawei.com/

Thanks,
Lingfeng

Powered by blists - more mailing lists