Re: [PATCH v4 21/21] NFS: Do uncached readdir when we're seeking a cookie in an empty page cache

[Trond Myklebust <trondmy@hammerspace.com>]

On Tue, 2020-11-10 at 09:48 -0500, David Wysochanski wrote:
> On Sat, Nov 7, 2020 at 9:14 AM <trondmy@kernel.org> wrote:
> > 
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > If the directory is changing, causing the page cache to get
> > invalidated
> > while we are listing the contents, then the NFS client is currently
> > forced
> > to read in the entire directory contents from scratch, because it
> > needs
> > to perform a linear search for the readdir cookie. While this is
> > not
> > an issue for small directories, it does not scale to directories
> > with
> > millions of entries.
> > In order to be able to deal with large directories that are
> > changing,
> > add a heuristic to ensure that if the page cache is empty, and we
> > are
> > searching for a cookie that is not the zero cookie, we just default
> > to
> > performing uncached readdir.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/dir.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index 238872d116f7..d7a9efd31ecd 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -917,11 +917,28 @@ static int find_and_lock_cache_page(struct
> > nfs_readdir_descriptor *desc)
> >         return res;
> >  }
> > 
> > +static bool nfs_readdir_dont_search_cache(struct
> > nfs_readdir_descriptor *desc)
> > +{
> > +       struct address_space *mapping = desc->file->f_mapping;
> > +       struct inode *dir = file_inode(desc->file);
> > +       unsigned int dtsize = NFS_SERVER(dir)->dtsize;
> > +       loff_t size = i_size_read(dir);
> > +
> > +       /*
> > +        * Default to uncached readdir if the page cache is empty,
> > and
> > +        * we're looking for a non-zero cookie in a large
> > directory.
> > +        */
> > +       return desc->dir_cookie != 0 && mapping->nrpages == 0 &&
> > size > dtsize;
> > +}
> > +
> >  /* Search for desc->dir_cookie from the beginning of the page
> > cache */
> >  static int readdir_search_pagecache(struct nfs_readdir_descriptor
> > *desc)
> >  {
> >         int res;
> > 
> > +       if (nfs_readdir_dont_search_cache(desc))
> > +               return -EBADCOOKIE;
> > +
> >         do {
> >                 if (desc->page_index == 0) {
> >                         desc->current_index = 0;
> > --
> > 2.28.0
> > 
> I did a lot of testing yesterday and last night and this mostly
> behaves as designed.
> 
> However, before you sent this I was starting to test the following
> patch which adds a NFS_DIR_CONTEXT_UNCACHED
> flag inside nfs_open_dir_context.  I was not sure about the logic
> when
> to turn it on, so for now I'd ignore that
> (especially nrpages > NFS_READDIR_UNCACHED_THRESHOLD).  However, I'm
> curious why:
> 1. you didn't take the approach of adding a per-process context flag
> so once a process hits this condition, the
> process would just shift to uncached and be unaffected by any other
> process.  I wonder about multiple directory
> listing processes defeating this logic if it's not per-process so we
> may get an unbounded time still
> 2. you put the logic inside readdir_search_pagecache rather than
> inside the calling do { .. } while loop

The reason for using uncached readdir here is because we're having
trouble sharing the cache. However if there is a possibility to do so,
because we have multiple processes racing to read the same directory,
then why should we not try?

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