Re: [PATCH v6 05/13] NFS: Improve algorithm for falling back to uncached readdir

From: Trond Myklebust <trondmy@hammerspace.com>
To: "bcodding@redhat.com" <bcodding@redhat.com>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v6 05/13] NFS: Improve algorithm for falling back to uncached readdir
Date: Tue, 22 Feb 2022 20:11:13 +0000	[thread overview]
Message-ID: <4f1963e80028d6c162512465cee403c10fa2ab77.camel@hammerspace.com> (raw)
In-Reply-To: <17D70218-EECB-456B-9BCA-E7FC128A4864@redhat.com>

On Tue, 2022-02-22 at 07:50 -0500, Benjamin Coddington wrote:
> On 21 Feb 2022, at 18:20, Trond Myklebust wrote:
> 
> > On Mon, 2022-02-21 at 16:10 -0500, Benjamin Coddington wrote:
> > > On 21 Feb 2022, at 15:55, Trond Myklebust wrote:
> > > > 
> > > > We will always need the ability to cut over to uncached
> > > > readdir.
> > > 
> > > Yes.
> > > 
> > > > If the cookie is no longer returned by the server because one
> > > > or more
> > > > files were deleted then we need to resolve the situation
> > > > somehow (IOW:
> > > > the 'rm *' case). The new algorithm _does_ improve performance
> > > > on those
> > > > situations, because it no longer requires us to read the entire
> > > > directory before switching over: we try 5 times, then fail
> > > > over.
> > > 
> > > Yes, using per-page validation doesn't remove the need for
> > > uncached
> > > readdir.  It does allow a reader to simply resume filling the
> > > cache where
> > > it left off.  There's no need to try 5 times and fail over.  And
> > > there's
> > > no need to make a trade-off and make the situation worse in
> > > certain
> > > scenarios.
> > > 
> > > I thought I'd point that out and make an offer to re-submit it. 
> > > Any
> > > interest?
> > > 
> > 
> > As I recall, I had concerns about that approach. Can you explain
> > again
> > how it will work?
> 
> Every page of readdir results has the directory's change attr stored
> on the
> page.  That, along with the page's index and the first cookie is
> enough
> information to determine if the page's data can be used by another
> process.
> 
> Which means that when the pagecache is dropped, fillers don't have to
> restart
> filling the cache at page index 0, they can continue to fill at
> whatever
> index they were at previously.  If another process finds a page that
> doesn't
> match its page index, cookie, and the current directory's change
> attr, the
> page is dropped and refilled from that process' indexing.
> 
> > A few of the concerns I have revolve around telldir()/seekdir(). If
> > the
> > platform is 32-bit, then we cannot use cookies as the telldir()
> > output,
> > and so our only option is to use offsets into the page cache (this
> > is
> > why this patch carves out an exception if desc->dir_cookie == 0).
> > How
> > would that work with you scheme?
> 
> For 32-bit seekdir, pages are filled starting at index 0.  This is
> very
> unlikely to match other readers (unless they also do the _same_
> seekdir).
> 
> > Even in the 64-bit case where are able to use cookies for
> > telldir()/seekdir(), how do we determine an appropriate page index
> > after a seekdir()?
> 
> We don't.  Instead we start filling at index 0.  Again, the pagecache
> will
> only be useful to other processes that have done the same llseek.
> 
> This approach optimizes the pagecache for processes that are doing
> similar
> work, and has the added benefit of scaling well for large directory
> listings
> under memory pressure.  Also a number of classes of directory
> modifications
> (such as renames, or insertions/deletions at locations a reader has
> moved
> beyond) are no longer a reason to re-fill the pagecache from scratch.
> 

OK, you've got me more or less sold on it.

I'd still like to figure out how to improve the performance for seekdir
(since I do have an interest in re-exporting NFS) but I've been playing
around with a couple of patches that implement your concept and they do
seem to work well for the common case of a linear read through the
directory.

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