Re: [PATCH v5 00/22] Readdir enhancements

From: David Wysochanski <dwysocha@redhat.com>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v5 00/22] Readdir enhancements
Date: Sat, 14 Nov 2020 08:32:18 -0500	[thread overview]
Message-ID: <CALF+zOnvne6xyKSTbzkYBcJXfeCKwgc9nmdo4Hzz60dUE-sJog@mail.gmail.com> (raw)
In-Reply-To: <6b07ff95824f5b46237fa07f5f72d8261d764007.camel@hammerspace.com>

On Thu, Nov 12, 2020 at 1:26 PM Trond Myklebust <trondmy@hammerspace.com> 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:
>
> ---
>  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);
>
>

If you want to pursue this let me know and I will run through some
tests with this patch (or some other patch) and all NFS versions.
I've mostly been testing with NFSv4 because that has been the
problematic case, but this would affect NFSv3 too.

I too had been looking at 07b5ce8ef2d8 because it causes NFSv3 to
behave differently than NFSv4 (I saw with NFSv3,
nfs_attribute_cache_expired() was always false while a directory is
being listed).  I pondered whether this was the right direction for
NFSv4 as well since that causes us to avoid dropping the pagecache and
kinda "neuters" it for a period of time while a large directory
listing was in progress.  However I didn't see a way to achieve the
NFSv3 behavior for NFSv4 and I wasn't convinced NFSv3 behavior was
necessarily the best approach.