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

[Su Hui]

On 2025/6/24 21:41, Chuck Lever wrote:
> On 6/23/25 9:31 PM, Su Hui wrote:
> > 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.
> Hi Su -
>
> Split the individual changes Neil suggested into separate patches. That
> makes the changes easier to review.

Hi,

Thanks for your remind. I will split these individual changes.
It won't be too long, at the latest this week I will submit this patchset.

Regards,
Su Hui