linux-kernel - Re: [PATCH] nfsd: Using guard() to simplify nfsd_cache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7975be21-045e-4b2b-9c73-79aba5b683db@nfschina.com>
Date: Tue, 24 Jun 2025 09:45:27 +0800
From: Su Hui <suhui@...china.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: chuck.lever@...cle.com, jlayton@...nel.org, neil@...wn.name,
 okorniev@...hat.com, Dai.Ngo@...cle.com, tom@...pey.com,
 linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel-janitors@...r.kernel.org, Su Hui <suhui@...china.com>
Subject: Re: [PATCH] nfsd: Using guard() to simplify nfsd_cache_lookup()

On 2025/6/23 23:47, Dan Carpenter wrote:
> On Mon, Jun 23, 2025 at 08:22:27PM +0800, 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.
>>
>> Signed-off-by: Su Hui <suhui@...china.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;
> It took me a while to figure out why we've added a goto here.  In the
> original code this "goto out;" was a "spin_unlock(&b->cache_lock);".
> The spin_unlock() is more readable because you can immediately see that
> it's trying to drop the lock where a "goto out;" is less obvious about
> the intention.

Does "break;" be better in this place?  Meaning Break this lock guard scope.

But as NeillBrown suggestion[1], this patch will be replaced by several 
patches.

No matter what, this "goto out;" will be removed in the next v2 patchset.

> I think this patch works fine, but I'm not sure it's an improvement.

Got it, thanks for your suggestions!

[1] 
https://lore.kernel.org/all/175072435698.2280845.12079422273351211469@noble.neil.brown.name/

regards,
Su Hui