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: <99a15559-072d-4c45-b12f-b42b4884cb06@nfschina.com>
Date: Tue, 24 Jun 2025 22:21:55 +0800
From: Su Hui <suhui@...china.com>
To: Chuck Lever <chuck.lever@...cle.com>, NeilBrown <neil@...wn.name>
Cc: jlayton@...nel.org, 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/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