[v2] nfsd: remove long-standing revoked delegations by force

[PATCH v2] nfsd: remove long-standing revoked delegations by force

Posted by Li Lingfeng 4 weeks, 1 day ago

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 like this:
nfs4_laundromat                       nfsd4_delegreturn
 unhash_delegation_locked
 list_add // add dp to reaplist
          // by dl_recall_lru
 list_del_init // delete dp from
               // reaplist
                                       destroy_delegation
                                        unhash_delegation_locked
                                         // do nothing but return false
 revoke_delegation
 list_add // add dp to cl_revoked
          // by dl_recall_lru

The delegation will remain in the server's cl_revoked list while 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.

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@huawei.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@huawei.com>
---
  Changes in v2:
  1) Set SC_STATUS_CLOSED unconditionally in destroy_delegation();
  2) Determine whether to remove the delegation based on SC_STATUS_CLOSED,
     rather than by timeout;
  3) Modify the commit message.
 fs/nfsd/nfs4state.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 88c347957da5..bb9e1df4e41f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1336,6 +1336,11 @@ static void destroy_delegation(struct nfs4_delegation *dp)
 
 	spin_lock(&state_lock);
 	unhashed = unhash_delegation_locked(dp, SC_STATUS_CLOSED);
+	/*
+	 * Unconditionally set SC_STATUS_CLOSED, regardless of whether the
+	 * delegation is hashed, to mark the current delegation as invalid.
+	 */
+	dp->dl_stid.sc_status |= SC_STATUS_CLOSED;
 	spin_unlock(&state_lock);
 	if (unhashed)
 		destroy_unhashed_deleg(dp);
@@ -4326,6 +4331,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 +4477,19 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	default:
 		seq->status_flags = 0;
 	}
+	if (!list_empty(&clp->cl_revoked)) {
+		spin_lock(&clp->cl_lock);
+		list_for_each_safe(pos, next, &clp->cl_revoked) {
+			dp = list_entry(pos, struct nfs4_delegation, dl_recall_lru);
+			if (dp->dl_stid.sc_status & SC_STATUS_CLOSED) {
+				list_del_init(&dp->dl_recall_lru);
+				spin_unlock(&clp->cl_lock);
+				nfs4_put_stid(&dp->dl_stid);
+				spin_lock(&clp->cl_lock);
+			}
+		}
+		spin_unlock(&clp->cl_lock);
+	}
 	if (!list_empty(&clp->cl_revoked))
 		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
 	if (atomic_read(&clp->cl_admin_revoked))
-- 
2.46.1

Re: [PATCH v2] nfsd: remove long-standing revoked delegations by force

Posted by Jeff Layton 4 weeks, 1 day ago

On Wed, 2025-09-03 at 19:59 +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 like this:
> nfs4_laundromat                       nfsd4_delegreturn
>  unhash_delegation_locked
>  list_add // add dp to reaplist
>           // by dl_recall_lru
>  list_del_init // delete dp from
>                // reaplist
>                                        destroy_delegation
>                                         unhash_delegation_locked
>                                          // do nothing but return false
>  revoke_delegation
>  list_add // add dp to cl_revoked
>           // by dl_recall_lru
> 
> The delegation will remain in the server's cl_revoked list while 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.
> 
> 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@huawei.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@huawei.com>
> ---
>   Changes in v2:
>   1) Set SC_STATUS_CLOSED unconditionally in destroy_delegation();
>   2) Determine whether to remove the delegation based on SC_STATUS_CLOSED,
>      rather than by timeout;
>   3) Modify the commit message.
>  fs/nfsd/nfs4state.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 88c347957da5..bb9e1df4e41f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1336,6 +1336,11 @@ static void destroy_delegation(struct nfs4_delegation *dp)
>  
>  	spin_lock(&state_lock);
>  	unhashed = unhash_delegation_locked(dp, SC_STATUS_CLOSED);
> +	/*
> +	 * Unconditionally set SC_STATUS_CLOSED, regardless of whether the
> +	 * delegation is hashed, to mark the current delegation as invalid.
> +	 */
> +	dp->dl_stid.sc_status |= SC_STATUS_CLOSED;
>  	spin_unlock(&state_lock);
>  	if (unhashed)
>  		destroy_unhashed_deleg(dp);
> @@ -4326,6 +4331,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;
>  

nit: These could be moved inside the if statement below.

>  	if (resp->opcnt != 1)
>  		return nfserr_sequence_pos;
> @@ -4470,6 +4477,19 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	default:
>  		seq->status_flags = 0;
>  	}

I wouldn't mind a comment here that explains why we have to do this.
This is the sort of thing that will have us all scratching our heads in
a few years.

> +	if (!list_empty(&clp->cl_revoked)) {
> +		spin_lock(&clp->cl_lock);
> +		list_for_each_safe(pos, next, &clp->cl_revoked) {
> +			dp = list_entry(pos, struct nfs4_delegation, dl_recall_lru);
> +			if (dp->dl_stid.sc_status & SC_STATUS_CLOSED) {
> +				list_del_init(&dp->dl_recall_lru);
> +				spin_unlock(&clp->cl_lock);
> +				nfs4_put_stid(&dp->dl_stid);
> +				spin_lock(&clp->cl_lock);
> +			}
> +		}
> +		spin_unlock(&clp->cl_lock);
> +	}

nit: I'd move the if statement below inside the above if statement. No
need to check list_empty() twice if it was empty the first time. Maybe
the compiler papers over this and only does it once?

>  	if (!list_empty(&clp->cl_revoked))
>  		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
>  	if (atomic_read(&clp->cl_admin_revoked))

Otherwise, this looks great. Thanks for the patch!

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

Re: [PATCH v2] nfsd: remove long-standing revoked delegations by force

Posted by Chuck Lever 4 weeks ago

On 9/3/25 8:15 AM, Jeff Layton wrote:
> On Wed, 2025-09-03 at 19:59 +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 like this:
>> nfs4_laundromat                       nfsd4_delegreturn
>>  unhash_delegation_locked
>>  list_add // add dp to reaplist
>>           // by dl_recall_lru
>>  list_del_init // delete dp from
>>                // reaplist
>>                                        destroy_delegation
>>                                         unhash_delegation_locked
>>                                          // do nothing but return false
>>  revoke_delegation
>>  list_add // add dp to cl_revoked
>>           // by dl_recall_lru
>>
>> The delegation will remain in the server's cl_revoked list while 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.
>>
>> 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@huawei.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@huawei.com>
>> ---
>>   Changes in v2:
>>   1) Set SC_STATUS_CLOSED unconditionally in destroy_delegation();
>>   2) Determine whether to remove the delegation based on SC_STATUS_CLOSED,
>>      rather than by timeout;
>>   3) Modify the commit message.
>>  fs/nfsd/nfs4state.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 88c347957da5..bb9e1df4e41f 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1336,6 +1336,11 @@ static void destroy_delegation(struct nfs4_delegation *dp)
>>  
>>  	spin_lock(&state_lock);
>>  	unhashed = unhash_delegation_locked(dp, SC_STATUS_CLOSED);
>> +	/*
>> +	 * Unconditionally set SC_STATUS_CLOSED, regardless of whether the
>> +	 * delegation is hashed, to mark the current delegation as invalid.
>> +	 */
>> +	dp->dl_stid.sc_status |= SC_STATUS_CLOSED;
>>  	spin_unlock(&state_lock);
>>  	if (unhashed)
>>  		destroy_unhashed_deleg(dp);
>> @@ -4326,6 +4331,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;
>>  
> 
> nit: These could be moved inside the if statement below.
> 
>>  	if (resp->opcnt != 1)
>>  		return nfserr_sequence_pos;
>> @@ -4470,6 +4477,19 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>  	default:
>>  		seq->status_flags = 0;
>>  	}
> 
> I wouldn't mind a comment here that explains why we have to do this.
> This is the sort of thing that will have us all scratching our heads in
> a few years.
> 
>> +	if (!list_empty(&clp->cl_revoked)) {
>> +		spin_lock(&clp->cl_lock);
>> +		list_for_each_safe(pos, next, &clp->cl_revoked) {
>> +			dp = list_entry(pos, struct nfs4_delegation, dl_recall_lru);
>> +			if (dp->dl_stid.sc_status & SC_STATUS_CLOSED) {
>> +				list_del_init(&dp->dl_recall_lru);
>> +				spin_unlock(&clp->cl_lock);
>> +				nfs4_put_stid(&dp->dl_stid);
>> +				spin_lock(&clp->cl_lock);
>> +			}
>> +		}
>> +		spin_unlock(&clp->cl_lock);
>> +	}
> 
> nit: I'd move the if statement below inside the above if statement. No
> need to check list_empty() twice if it was empty the first time. Maybe
> the compiler papers over this and only does it once?
> 
>>  	if (!list_empty(&clp->cl_revoked))
>>  		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
>>  	if (atomic_read(&clp->cl_admin_revoked))
> 
> Otherwise, this looks great. Thanks for the patch!
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Li, I'm assuming you are going to address Jeff's additional comments
here and send another revision of this patch. So I'm waiting for
another version... let me know if you plan not to send one.


-- 
Chuck Lever

Re: [PATCH v2] nfsd: remove long-standing revoked delegations by force

Posted by Li Lingfeng 4 weeks ago

在 2025/9/4 22:08, Chuck Lever 写道:
> On 9/3/25 8:15 AM, Jeff Layton wrote:
>> On Wed, 2025-09-03 at 19:59 +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 like this:
>>> nfs4_laundromat                       nfsd4_delegreturn
>>>   unhash_delegation_locked
>>>   list_add // add dp to reaplist
>>>            // by dl_recall_lru
>>>   list_del_init // delete dp from
>>>                 // reaplist
>>>                                         destroy_delegation
>>>                                          unhash_delegation_locked
>>>                                           // do nothing but return false
>>>   revoke_delegation
>>>   list_add // add dp to cl_revoked
>>>            // by dl_recall_lru
>>>
>>> The delegation will remain in the server's cl_revoked list while 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.
>>>
>>> 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@huawei.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@huawei.com>
>>> ---
>>>    Changes in v2:
>>>    1) Set SC_STATUS_CLOSED unconditionally in destroy_delegation();
>>>    2) Determine whether to remove the delegation based on SC_STATUS_CLOSED,
>>>       rather than by timeout;
>>>    3) Modify the commit message.
>>>   fs/nfsd/nfs4state.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 88c347957da5..bb9e1df4e41f 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -1336,6 +1336,11 @@ static void destroy_delegation(struct nfs4_delegation *dp)
>>>   
>>>   	spin_lock(&state_lock);
>>>   	unhashed = unhash_delegation_locked(dp, SC_STATUS_CLOSED);
>>> +	/*
>>> +	 * Unconditionally set SC_STATUS_CLOSED, regardless of whether the
>>> +	 * delegation is hashed, to mark the current delegation as invalid.
>>> +	 */
>>> +	dp->dl_stid.sc_status |= SC_STATUS_CLOSED;
>>>   	spin_unlock(&state_lock);
>>>   	if (unhashed)
>>>   		destroy_unhashed_deleg(dp);
>>> @@ -4326,6 +4331,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;
>>>   
>> nit: These could be moved inside the if statement below.
>>
>>>   	if (resp->opcnt != 1)
>>>   		return nfserr_sequence_pos;
>>> @@ -4470,6 +4477,19 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>   	default:
>>>   		seq->status_flags = 0;
>>>   	}
>> I wouldn't mind a comment here that explains why we have to do this.
>> This is the sort of thing that will have us all scratching our heads in
>> a few years.
>>
>>> +	if (!list_empty(&clp->cl_revoked)) {
>>> +		spin_lock(&clp->cl_lock);
>>> +		list_for_each_safe(pos, next, &clp->cl_revoked) {
>>> +			dp = list_entry(pos, struct nfs4_delegation, dl_recall_lru);
>>> +			if (dp->dl_stid.sc_status & SC_STATUS_CLOSED) {
>>> +				list_del_init(&dp->dl_recall_lru);
>>> +				spin_unlock(&clp->cl_lock);
>>> +				nfs4_put_stid(&dp->dl_stid);
>>> +				spin_lock(&clp->cl_lock);
>>> +			}
>>> +		}
>>> +		spin_unlock(&clp->cl_lock);
>>> +	}
>> nit: I'd move the if statement below inside the above if statement. No
>> need to check list_empty() twice if it was empty the first time. Maybe
>> the compiler papers over this and only does it once?
>>
>>>   	if (!list_empty(&clp->cl_revoked))
>>>   		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
>>>   	if (atomic_read(&clp->cl_admin_revoked))
>> Otherwise, this looks great. Thanks for the patch!
>>
>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Li, I'm assuming you are going to address Jeff's additional comments
> here and send another revision of this patch. So I'm waiting for
> another version... let me know if you plan not to send one.
>
Thank you for the reminder. I will send a new patch soon.

Thanks,
Lingfeng