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@xxxxxxxxxxxxxxx
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
