Message-ID: <d38db7cd-7e2c-412d-8d41-8deb938670e6@huawei.com>
Date: Wed, 3 Sep 2025 09:34:58 +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 22:29, Jeff Layton 写道:
> On Tue, 2025-09-02 at 22:21 +0800, Li Lingfeng wrote:
>> 在 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?
>>
> I can't think of any workarounds other than turning off delegations
> altogether.
>
> I guess your concern is that TEST_STATEID and FREE_STATEID would return
> BAD_STATEID in this case, even though the entry was still (technically)
> on the cl_revoked list? That seems like correct behavior. The client
> did send DELEGRETURN, after all.
Sorry for the noice. I was mistaken.
I was originally concerned that after setting SC_STATUS_CLOSED via
nfsd4_delegreturn --> destroy_delegation, if the client initiated a
FREE_STATEID, the server would skip the SC_TYPE_DELEG branch in
nfsd4_free_stateid() that would normally be executed.
However, in reality, after the client completes DELEGRETURN, it will not
perform FREE_STATEID for this delegation again since the delegation is
no longer in server->delegations.

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