Re: [PATCH] nfsd: Using guard() to simplify nfsd_cache_lookup()

[Su Hui <suhui@nfschina.com>]

On 2025/6/24 08:19, NeilBrown wrote:
> On Mon, 23 Jun 2025, Su Hui wrote:
>> Using guard() to replace *unlock* label. guard() makes lock/unlock code
>> more clear. Change the order of the code to let all lock code in the
>> same scope. No functional changes.
> While I agree that this code could usefully be cleaned up and that you
> have made some improvements, I think the use of guard() is a nearly
> insignificant part of the change.  You could easily do exactly the same
> patch without using guard() but having and explicit spin_unlock() before
> the new return.  That doesn't mean you shouldn't use guard(), but it
> does mean that the comment explaining the change could be more usefully
> focused on the "Change the order ..." part, and maybe explain what that
> is important.
Got it. I will focus on "Change the order ..." part in the next v2 patch.
> I actually think there is room for other changes which would make the
> code even better:
> - Change nfsd_prune_bucket_locked() to nfsd_prune_bucket().  Have it
>    take the lock when needed, then drop it, then call
>    nfsd_cacherep_dispose() - and return the count.
> - change nfsd_cache_insert to also skip updating the chain length stats
>    when it finds a match - in that case the "entries" isn't a chain
>    length. So just  lru_put_end(), return.  Have it return NULL if
>    no match was found
> - after the found_entry label don't use nfsd_reply_cache_free_locked(),
>    just free rp.  It has never been included in any rbtree or list, so it
>    doesn't need to be removed.
> - I'd be tempted to have nfsd_cache_insert() take the spinlock itself
>    and call it under rcu_read_lock() - and use RCU to free the cached
>    items.
> - put the chunk of code after the found_entry label into a separate
>    function and instead just return RC_REPLY (and maybe rename that
>    RC_CACHED).  Then in nfsd_dispatch(), if RC_CACHED was returned, call
>    that function that has the found_entry code.
>
> I think that would make the code a lot easier to follow.  Would you like
> to have a go at that - I suspect it would be several patches - or shall
> I do it?
>
> Thanks,
> NeilBrown
>
Really thanks for your suggestions!
Yes, I'd like to do it in the next v2 patchset as soon as possible.
I'm always searching some things I can participate in about linux kernel
community, so it's happy for me to do this thing.

regards,
Su Hui

>
>> Signed-off-by: Su Hui <suhui@nfschina.com>
>> ---
>>   fs/nfsd/nfscache.c | 99 ++++++++++++++++++++++------------------------
>>   1 file changed, 48 insertions(+), 51 deletions(-)
>>
>> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
>> index ba9d326b3de6..2d92adf3e6b0 100644
>> --- a/fs/nfsd/nfscache.c
>> +++ b/fs/nfsd/nfscache.c
>> @@ -489,7 +489,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp, unsigned int start,
>>   
>>   	if (type == RC_NOCACHE) {
>>   		nfsd_stats_rc_nocache_inc(nn);
>> -		goto out;
>> +		return rtn;
>>   	}
>>   
>>   	csum = nfsd_cache_csum(&rqstp->rq_arg, start, len);
>> @@ -500,64 +500,61 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp, unsigned int start,
>>   	 */
>>   	rp = nfsd_cacherep_alloc(rqstp, csum, nn);
>>   	if (!rp)
>> -		goto out;
>> +		return rtn;
>>   
>>   	b = nfsd_cache_bucket_find(rqstp->rq_xid, nn);
>> -	spin_lock(&b->cache_lock);
>> -	found = nfsd_cache_insert(b, rp, nn);
>> -	if (found != rp)
>> -		goto found_entry;
>> -	*cacherep = rp;
>> -	rp->c_state = RC_INPROG;
>> -	nfsd_prune_bucket_locked(nn, b, 3, &dispose);
>> -	spin_unlock(&b->cache_lock);
>> +	scoped_guard(spinlock, &b->cache_lock) {
>> +		found = nfsd_cache_insert(b, rp, nn);
>> +		if (found == rp) {
>> +			*cacherep = rp;
>> +			rp->c_state = RC_INPROG;
>> +			nfsd_prune_bucket_locked(nn, b, 3, &dispose);
>> +			goto out;
>> +		}
>> +		/* We found a matching entry which is either in progress or done. */
>> +		nfsd_reply_cache_free_locked(NULL, rp, nn);
>> +		nfsd_stats_rc_hits_inc(nn);
>> +		rtn = RC_DROPIT;
>> +		rp = found;
>> +
>> +		/* Request being processed */
>> +		if (rp->c_state == RC_INPROG)
>> +			goto out_trace;
>> +
>> +		/* From the hall of fame of impractical attacks:
>> +		 * Is this a user who tries to snoop on the cache?
>> +		 */
>> +		rtn = RC_DOIT;
>> +		if (!test_bit(RQ_SECURE, &rqstp->rq_flags) && rp->c_secure)
>> +			goto out_trace;
>>   
>> +		/* Compose RPC reply header */
>> +		switch (rp->c_type) {
>> +		case RC_NOCACHE:
>> +			break;
>> +		case RC_REPLSTAT:
>> +			xdr_stream_encode_be32(&rqstp->rq_res_stream, rp->c_replstat);
>> +			rtn = RC_REPLY;
>> +			break;
>> +		case RC_REPLBUFF:
>> +			if (!nfsd_cache_append(rqstp, &rp->c_replvec))
>> +				return rtn; /* should not happen */
>> +			rtn = RC_REPLY;
>> +			break;
>> +		default:
>> +			WARN_ONCE(1, "nfsd: bad repcache type %d\n", rp->c_type);
>> +		}
>> +
>> +out_trace:
>> +		trace_nfsd_drc_found(nn, rqstp, rtn);
>> +		return rtn;
>> +	}
>> +out:
>>   	nfsd_cacherep_dispose(&dispose);
>>   
>>   	nfsd_stats_rc_misses_inc(nn);
>>   	atomic_inc(&nn->num_drc_entries);
>>   	nfsd_stats_drc_mem_usage_add(nn, sizeof(*rp));
>> -	goto out;
>> -
>> -found_entry:
>> -	/* We found a matching entry which is either in progress or done. */
>> -	nfsd_reply_cache_free_locked(NULL, rp, nn);
>> -	nfsd_stats_rc_hits_inc(nn);
>> -	rtn = RC_DROPIT;
>> -	rp = found;
>> -
>> -	/* Request being processed */
>> -	if (rp->c_state == RC_INPROG)
>> -		goto out_trace;
>> -
>> -	/* From the hall of fame of impractical attacks:
>> -	 * Is this a user who tries to snoop on the cache? */
>> -	rtn = RC_DOIT;
>> -	if (!test_bit(RQ_SECURE, &rqstp->rq_flags) && rp->c_secure)
>> -		goto out_trace;
>> -
>> -	/* Compose RPC reply header */
>> -	switch (rp->c_type) {
>> -	case RC_NOCACHE:
>> -		break;
>> -	case RC_REPLSTAT:
>> -		xdr_stream_encode_be32(&rqstp->rq_res_stream, rp->c_replstat);
>> -		rtn = RC_REPLY;
>> -		break;
>> -	case RC_REPLBUFF:
>> -		if (!nfsd_cache_append(rqstp, &rp->c_replvec))
>> -			goto out_unlock; /* should not happen */
>> -		rtn = RC_REPLY;
>> -		break;
>> -	default:
>> -		WARN_ONCE(1, "nfsd: bad repcache type %d\n", rp->c_type);
>> -	}
>> -
>> -out_trace:
>> -	trace_nfsd_drc_found(nn, rqstp, rtn);
>> -out_unlock:
>> -	spin_unlock(&b->cache_lock);
>> -out:
>>   	return rtn;
>>   }
>>   
>> -- 
>> 2.30.2
>>
>>