Re: [PATCH v5 3/5] nfsd: rework refcounting in filecache

From: Chuck Lever III <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Neil Brown <neilb@suse.de>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v5 3/5] nfsd: rework refcounting in filecache
Date: Tue, 1 Nov 2022 19:13:23 +0000	[thread overview]
Message-ID: <0B609D3B-955F-4F15-9EDD-16B98087EF06@oracle.com> (raw)
In-Reply-To: <53599736e1733bd011325170a269e9adda2f2de1.camel@kernel.org>

> On Nov 1, 2022, at 2:57 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Tue, 2022-11-01 at 14:03 -0400, Jeff Layton wrote:
>> On Tue, 2022-11-01 at 17:25 +0000, Chuck Lever III wrote:
>>> 
>>> 
>>> Test results:
>>> 
>>> When I run my test I "watch cat /proc/fs/nfsd/filecache". The
>>> workload is 12-thread "untar git && make git && make test" on
>>> NFSv3. It's showing worse eviction behavior than the current
>>> code.
>>> 
>> 
>> What do you mean by "worse" here?
>> 
>>> Basically all cached items appear to be immediately placed on
>>> the LRU. Do you expect this behavior change? We want to keep
>>> the LRU as short as possible; but maybe the LRU callback is
>>> stopping after a few items, so it might not matter.
>>> 
>> 
>> Could be. I'm not sure how that works.
>> 
> 
> Looking more at the old LRU code, I'm not sure we can make a direct
> comparison on behavior. I think that the old code was just broken, and
> that it inappropriately took entries off the list when it shouldn't
> have. That mostly worked out in the end, but I don't think the lifetime
> of those entries was what was wanted or expected.
> 
> With the new code, it's much more clear. The only entries on the LRU are
> GC entries with no active references. Once nfsd_file_do_acquire is
> called, the entry comes off the LRU.

The new mechanism is not working the way you might have intended. I see
the "total" and "LRU" numbers equaling each other throughout the test
run, which is a sign the LRU is not working correctly. What you said
here suggests that the "LRU" number is supposed to be less than the
total number of cached items, and that's the way the old code behaved.

> I don't see how we can make the LRU any shorter.

I'll dig into it and see what's going on.

--
Chuck Lever