Re: [PATCH 4/5] NFS: Further optimisations for 'ls -l'

[Trond Myklebust <trondmy@hammerspace.com>]

On Wed, 2021-09-29 at 16:21 -0400, Benjamin Coddington wrote:
> On 29 Sep 2021, at 12:23, Trond Myklebust wrote:
> 
> > On Wed, 2021-09-29 at 10:56 -0400, Benjamin Coddington wrote:
> > > On 29 Sep 2021, at 9:49, trondmy@kernel.org wrote:
> > > 
> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > 
> > > > If a user is doing 'ls -l', we have a heuristic in GETATTR that
> > > > tells
> > > > the readdir code to try to use READDIRPLUS in order to refresh
> > > > the
> > > > inode
> > > > attributes. In certain cirumstances, we also try to invalidate
> > > > the
> > > > remaining directory entries in order to ensure this refresh.
> > > > 
> > > > If there are multiple readers of the directory, we probably
> > > > should
> > > > avoid
> > > > invalidating the page cache, since the heuristic breaks down in
> > > > that
> > > > situation anyway.
> > > 
> > > Hi Trond, I'm curious about the motivation for this work because
> > > we're often
> > > managing expectations about performance between various
> > > workloads. 
> > > What
> > > does the workload look like that prompted you to make this
> > > optimization?
> > > 
> > > I'm also interested because I have some work that improves
> > > readdir
> > > performance for multiple readers on large directories, but is
> > > rotting
> > > because things have been "good enough" for folks over here.
> > > 
> > 
> > Are you talking about this patch specifically, or the series in
> > general?
> 
> A bit of both - the first two patches didn't really make sense to me,
> but I
> get it now.  Thanks.
> 
> > The general motivation for the series is that in certain
> > circumstances
> > involving a read-only scenario (no changes to the directory) and
> > multiple processes we're seeing a regression w.r.t. older kernels.
> > 
> > The motivation for this particular patch is twofold:
> >    1. I want to get rid of the cached page_index in the inode and
> >       replace it with something less persistent (inodes are
> > forever,
> >       unlike file descriptors).
> >    2. The heuristic in question is designed to only work in the
> > single
> >       process/single threaded case.
> 
> Make sense to me now.
> 
> I'm wondering if this might be creating a READDIR op amplification
> for N
> readers sharing the directory's fd because one process can be
> dropping the
> cache, which artificially deflates desc->page_index for a given
> dir_cookie.
> That lower page_index gets reused on the next pass dropping the
> cache, and
> it'll keep using the bottom pages and doing READDIRs.  That wasn't
> possible
> before because we only invalidated past nfsi->page_index.
> 
> Maybe an unlikely case (but I think some http servers could behave
> this
> way), and I'd have to test it to be sure.  I can try to do that
> unless you
> think its not possible or concerning.
> 

It is concerning, and indeed in our test we are seeing READDIR
amplification with these multiple process accesses. So scenarios like
the one you describe above are exactly the kind of issue I was looking
to fix with these patches.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com