> On Oct 27, 2022, at 6:55 PM, NeilBrown <neilb@xxxxxxx> wrote:
>
> On Fri, 28 Oct 2022, Jeff Layton wrote:
>> On Fri, 2022-10-28 at 09:20 +1100, NeilBrown wrote:
>>> On Fri, 28 Oct 2022, Jeff Layton wrote:
>>>> Currently, nfsd_files live on the LRU once they are added until they are
>>>> unhashed. There's no need to keep ones that are actively in use there.
>>>
>>> Is that true?
>>> nfsd_file_do_acquire() calls nfsd_file_lru_remove()
>>> Isn't that enough to keep the file off the lru while it is active?
>>>
>>> Thanks,
>>> NeilBrown
>>>
>>
>> After patch #1, it doesn't call that anymore. That's probably a (minor)
>> regression then.
>
> Yes, I eventually found that - thanks.
>
>>
>> After patch #1, the LRU holds a reference. If you successfully remove it
>> from the LRU, you need to transfer or put that reference. Doing the LRU
>> handling in the get and put routines seems more natural, I think.
>
> Maybe. But then you need a __get as well as a get.
> Though it might seem asymmetric, I would prefer removing from the lru in
> 'acquire' and adding to the lru in put.
That's exactly the design introduced by commit 4a0e73e635e3
("NFSD: Leave open files out of the filecache LRU"). I also
would like to keep that behavior -- that's what a real LRU
is for.
>> Maybe I just need to squash this patch into #1?
>
> Or do the "put" if lru_remove succeeds in the first patch. Then revise
> it all in the second.
>
> Thanks,
> NeilBrown
>
>
>>
>>>
>>>>
>>>> Before incrementing the refcount, do a lockless check for nf_lru being
>>>> empty. If it's not then attempt to remove the entry from the LRU. If
>>>> that's successful, claim the LRU reference and return it. If the removal
>>>> fails (or if the list_head was empty), then just increment the counter
>>>> as we normally would.
>>>>
>>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>>> ---
>>>> fs/nfsd/filecache.c | 23 ++++++++++++++++++++---
>>>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>>> index e63534f4b9f8..d2bbded805d4 100644
>>>> --- a/fs/nfsd/filecache.c
>>>> +++ b/fs/nfsd/filecache.c
>>>> @@ -420,14 +420,31 @@ nfsd_file_unhash(struct nfsd_file *nf)
>>>> return false;
>>>> }
>>>>
>>>> -struct nfsd_file *
>>>> -nfsd_file_get(struct nfsd_file *nf)
>>>> +static struct nfsd_file *
>>>> +__nfsd_file_get(struct nfsd_file *nf)
>>>> {
>>>> if (likely(refcount_inc_not_zero(&nf->nf_ref)))
>>>> return nf;
>>>> return NULL;
>>>> }
>>>>
>>>> +struct nfsd_file *
>>>> +nfsd_file_get(struct nfsd_file *nf)
>>>> +{
>>>> + /*
>>>> + * Do a lockless list_empty check first, before attempting to
>>>> + * remove it, so we can avoid the spinlock when it's not on the
>>>> + * list.
>>>> + *
>>>> + * If we successfully remove it from the LRU, then we can just
>>>> + * claim the LRU reference and return it. Otherwise, we need to
>>>> + * bump the counter the old-fashioned way.
>>>> + */
>>>> + if (!list_empty(&nf->nf_lru) && nfsd_file_lru_remove(nf))
>>>> + return nf;
>>>> + return __nfsd_file_get(nf);
>>>> +}
>>>> +
>>>> /**
>>>> * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list
>>>> * @nf: nfsd_file to be unhashed and queued
>>>> @@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose)
>>>> * to take a reference. If that fails, just ignore
>>>> * the file altogether.
>>>> */
>>>> - if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf))
>>>> + if (!nfsd_file_lru_remove(nf) && !__nfsd_file_get(nf))
>>>> return false;
>>>> list_add(&nf->nf_lru, dispose);
>>>> return true;
>>>> --
>>>> 2.37.3
>>>>
>>>>
>>
>> --
>> Jeff Layton <jlayton@xxxxxxxxxx>
>>
--
Chuck Lever
