Message-ID: <c5869e23-5294-4757-a757-868748b3bc65@huawei.com>
Date: Tue, 2 Sep 2025 22:21:36 +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


在 2025/9/2 21:40, Jeff Layton 写道:
> On Tue, 2025-09-02 at 20:10 +0800, Li Lingfeng wrote:
>> 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);
>>>> +			}
>>>> +		}
> FYI: this list is protected by the clp->cl_lock. You need to hold it to
> do this list walk.
>
>>>> +	}
>>>>    	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
> Ok, I get the problem, but I still disagree with the solution. I don't
> think we can just time these things out. Ideally we'd close the race
> window, but the sc_status field is protected by the global state_lock
> and I don't think we want to take it in revoke_delegation.
>
> The best solution I can see is to have destroy_delegation()
> unconditionally set SC_STATUS_CLOSED, and then you can do the list walk
> above, but checking for that flag instead of testing for a timeout.
This might potentially affect the normal TEST_STATEID/FREE_STATEID flow,
as nfsd4_free_stateid() branches differently based on whether
SC_STATUS_CLOSED is set. Alternatively, I was wondering if you could
suggest a workaround to avoid this issue?

Thanks,
Lingfeng

>
> I'm still not thrilled with this solution though. It makes SEQUENCE a
> bit more heavyweight than I'd like. I'm starting to think that we need
> to rework the overall delegation locking, but that's an ugly problem to
> tackle.

Powered by blists - more mailing lists