> On Jan 5, 2023, at 9:29 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Thu, 2023-01-05 at 14:18 +0000, Chuck Lever III wrote:
>> Hi Jeff-
>>
>>
>>> On Jan 5, 2023, at 7:15 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>>>
>>> Since v4 files are expected to be long-lived, there's little value in
>>> closing them out of the cache when there is conflicting access.
>>
>> Seems like targeting long-lived nfsd_file items could actually
>> be a hazardous behavior. Are you sure it's safe to leave it in
>> stable kernels?
>>
>
> Basically it just means we end up doing more opens than are needed in
> this situation.
>
> The notifiers just unnecessarily unhash a nfsd_file in this case, even
> though it doesn't have any chance of freeing it until the client issues
> a CLOSE, due to the persistent references held by the stateids.
>
> So, this really is just an optimization and not a "real bug".
Good to know. Let's add this to the patch description when you
redrive...?
>>> Rename NFSD_FILE_KEY_INODE to NFSD_FILE_KEY_INODE_GC,
>>
>> I'd prefer to keep the name as it is. It's for searching for
>> inodes, and adding the ".gc = true" to the search criteria is
>> enough to show what you are looking for.
>>
>
> Ok.
>
>>
>>> and change the
>>> comparator to also match the gc value in the key. Change both of the
>>> current users of that key to set the gc value in the key to "true".
>>>
>>> Also, test_bit returns bool, AFAICT, so I think we're ok to drop the
>>> !! from the condition later in the comparator.
>>
>> And I'd prefer that you leave this clean up for another patch.
>>
>
> Ok, I'll drop that hunk.
>
>>
>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>> ---
>>> fs/nfsd/filecache.c | 14 +++++++++-----
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index 9fff1fa09d08..a67b22579c6e 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -78,7 +78,7 @@ static struct rhashtable nfsd_file_rhash_tbl
>>> ____cacheline_aligned_in_smp;
>>>
>>> enum nfsd_file_lookup_type {
>>> - NFSD_FILE_KEY_INODE,
>>> + NFSD_FILE_KEY_INODE_GC,
>>> NFSD_FILE_KEY_FULL,
>>> };
>>>
>>> @@ -174,7 +174,9 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
>>> const struct nfsd_file *nf = ptr;
>>>
>>> switch (key->type) {
>>> - case NFSD_FILE_KEY_INODE:
>>> + case NFSD_FILE_KEY_INODE_GC:
>>> + if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
>>> + return 1;
>>> if (nf->nf_inode != key->inode)
>>> return 1;
>>> break;
>>> @@ -187,7 +189,7 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
>>> return 1;
>>> if (!nfsd_match_cred(nf->nf_cred, key->cred))
>>> return 1;
>>> - if (!!test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
>>> + if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
>>> return 1;
>>> if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0)
>>> return 1;
>>> @@ -681,8 +683,9 @@ static void
>>> nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
>>> {
>>> struct nfsd_file_lookup_key key = {
>>> - .type = NFSD_FILE_KEY_INODE,
>>> + .type = NFSD_FILE_KEY_INODE_GC,
>>> .inode = inode,
>>> + .gc = true,
>>> };
>>> struct nfsd_file *nf;
>>>
>>> @@ -1057,8 +1060,9 @@ bool
>>> nfsd_file_is_cached(struct inode *inode)
>>> {
>>> struct nfsd_file_lookup_key key = {
>>> - .type = NFSD_FILE_KEY_INODE,
>>> + .type = NFSD_FILE_KEY_INODE_GC,
>>> .inode = inode,
>>> + .gc = true,
>>> };
>>> bool ret = false;
>>>
>>> --
>>> 2.39.0
>>>
>>
>> --
>> Chuck Lever
>>
>>
>>
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
--
Chuck Lever
