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

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2022-11-01 at 19:13 +0000, Chuck Lever III wrote:
> 
> > 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've also watched that file while running xfstests on v3 and I do
occasionally see a small discrepancy between the two numbers, but they
track each other pretty closely.

That's more or less what I'd expect to see with NFSv3. In most cases
we're finding an entry for a READ/WRITE/COMMIT, using it, and then
putting it right away. GC'ed entries just live on the LRU most of the
time. We do take them off when we can, but we just can't that often.

That's ok though. Taking it off the LRU is still much cheaper than
opening a new struct file.

> > 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
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>