Re: [PATCH v5 00/22] Readdir enhancements

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2020-11-12 at 13:39 -0500, Benjamin Coddington wrote:
> 
> 
> On 12 Nov 2020, at 13:26, Trond Myklebust wrote:
> 
> > On Thu, 2020-11-12 at 16:51 +0000, Trond Myklebust wrote:
> > > 
> > > I was going to ask you if perhaps reverting Scott's commit
> > > 07b5ce8ef2d8
> > > ("NFS: Make nfs_readdir revalidate less often") might help here?
> > > My thinking is that will trigger more cache invalidations when
> > > the
> > > directory is changing underneath us, and will now trigger
> > > uncached
> > > readdir in those situations.
> > > 
> > > > 
> > 
> > IOW, the suggestion would be to apply something like the following
> > on
> > top of the existing readdir patchset:
> 
> I'm all for this approach, but - I'm rarely seeing the mapping-
> >nrpages == 0
> since the cache is dropped by a process in nfs_readdir() that
> immediately
> starts filling the cache again.

That's why I moved the check in readdir_search_pagecache. Unless that
process has set desc->dir_cookie == 0, then that should prevent the
refilling.

> It would make a lot more sense to me if we could do something like
> stash
> desc->page_index << PAGE_SHIFT in f_pos after each nfs_readdir, then
> the
> hueristic could check f_pos >> PAGE_SHIFT > nrpages.
> 
> Yes, f_pos is growing many different meanings in NFS directories,
> maybe we
> can stash it on the directory context.
> 
> Ben
> 
> > 
> > ---
> >  fs/nfs/dir.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index 3f70697729d8..384a4663f742 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -956,10 +956,10 @@ static int readdir_search_pagecache(struct
> > nfs_readdir_descriptor *desc)
> >  {
> >         int res;
> > 
> > -       if (nfs_readdir_dont_search_cache(desc))
> > -               return -EBADCOOKIE;
> > -
> >         do {
> > +               if (nfs_readdir_dont_search_cache(desc))
> > +                       return -EBADCOOKIE;
> > +
> >                 if (desc->page_index == 0) {
> >                         desc->current_index = 0;
> >                         desc->prev_index = 0;
> > @@ -1082,11 +1082,9 @@ static int nfs_readdir(struct file *file,
> > struct
> > dir_context *ctx)
> >          * to either find the entry with the appropriate number or
> >          * revalidate the cookie.
> >          */
> > -       if (ctx->pos == 0 || nfs_attribute_cache_expired(inode)) {
> > -               res = nfs_revalidate_mapping(inode, file-
> > >f_mapping);
> > -               if (res < 0)
> > -                       goto out;
> > -       }
> > +       res = nfs_revalidate_mapping(inode, file->f_mapping);
> > +       if (res < 0)
> > +               goto out;
> > 
> >         res = -ENOMEM;
> >         desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> > 
> > 
> > -- 
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@hammerspace.com
> 

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