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

[Su Hui]

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@xxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxxxxxxx/

regards,
Su Hui