Re: [PATCH v7 05/21] NFS: Store the change attribute in the directory page cache

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2022-02-24 at 09:53 -0500, Benjamin Coddington wrote:
> On 23 Feb 2022, at 16:12, trondmy@kernel.org wrote:
> 
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > Use the change attribute and the first cookie in a directory page 
> > cache
> > entry to validate that the page is up to date.
> > 
> > Suggested-by: Benjamin Coddington <bcodding@redhat.com>
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/dir.c | 68 
> > ++++++++++++++++++++++++++++------------------------
> >  1 file changed, 37 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index f2258e926df2..5d9367d9b651 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -139,6 +139,7 @@ struct nfs_cache_array_entry {
> >  };
> > 
> >  struct nfs_cache_array {
> > +       u64 change_attr;
> >         u64 last_cookie;
> >         unsigned int size;
> >         unsigned char page_full : 1,
> > @@ -175,7 +176,8 @@ static void nfs_readdir_array_init(struct 
> > nfs_cache_array *array)
> >         memset(array, 0, sizeof(struct nfs_cache_array));
> >  }
> > 
> > -static void nfs_readdir_page_init_array(struct page *page, u64 
> > last_cookie)
> > +static void nfs_readdir_page_init_array(struct page *page, u64 
> > last_cookie,
> > +                                       u64 change_attr)
> >  {
> >         struct nfs_cache_array *array;
> 
> 
> There's a hunk missing here, something like:
> 
> @@ -185,6 +185,7 @@ static void nfs_readdir_page_init_array(struct
> page 
> *page, u64 last_cookie,
>          nfs_readdir_array_init(array);
>          array->last_cookie = last_cookie;
>          array->cookies_are_ordered = 1;
> +       array->change_attr = change_attr;
>          kunmap_atomic(array);
>   }
> 
> > 
> > @@ -207,7 +209,7 @@ nfs_readdir_page_array_alloc(u64 last_cookie, 
> > gfp_t gfp_flags)
> >  {
> >         struct page *page = alloc_page(gfp_flags);
> >         if (page)
> > -               nfs_readdir_page_init_array(page, last_cookie);
> > +               nfs_readdir_page_init_array(page, last_cookie, 0);
> >         return page;
> >  }
> > 
> > @@ -304,19 +306,44 @@ int nfs_readdir_add_to_array(struct nfs_entry
> > *entry, struct page *page)
> >         return ret;
> >  }
> > 
> > +static bool nfs_readdir_page_cookie_match(struct page *page, u64 
> > last_cookie,
> > +                                         u64 change_attr)
> 
> How about "nfs_readdir_page_valid()"?  There's more going on than a 
> cookie match.
> 
> 
> > +{
> > +       struct nfs_cache_array *array = kmap_atomic(page);
> > +       int ret = true;
> > +
> > +       if (array->change_attr != change_attr)
> > +               ret = false;
> 
> Can we skip the next test if ret = false?

I'd expect the compiler to do that.

> 
> > +       if (array->size > 0 && array->array[0].cookie !=
> > last_cookie)
> > +               ret = false;
> > +       kunmap_atomic(array);
> > +       return ret;
> > +}
> > +
> > +static void nfs_readdir_page_unlock_and_put(struct page *page)
> > +{
> > +       unlock_page(page);
> > +       put_page(page);
> > +}
> > +
> >  static struct page *nfs_readdir_page_get_locked(struct
> > address_space 
> > *mapping,
> >                                                 pgoff_t index, u64
> > last_cookie)
> >  {
> >         struct page *page;
> > +       u64 change_attr;
> > 
> >         page = grab_cache_page(mapping, index);
> > -       if (page && !PageUptodate(page)) {
> > -               nfs_readdir_page_init_array(page, last_cookie);
> > -               if (invalidate_inode_pages2_range(mapping, index +
> > 1, -1) < 0)
> > -                       nfs_zap_mapping(mapping->host, mapping);
> > -               SetPageUptodate(page);
> > +       if (!page)
> > +               return NULL;
> > +       change_attr = inode_peek_iversion_raw(mapping->host);
> > +       if (PageUptodate(page)) {
> > +               if (nfs_readdir_page_cookie_match(page,
> > last_cookie,
> > +                                                 change_attr))
> > +                       return page;
> > +               nfs_readdir_clear_array(page);
> 
> 
> Why use i_version rather than nfs_save_change_attribute?  Seems
> having a
> consistent value across the pachecache and dir_verifiers would help
> debugging, and we've already have a bunch of machinery around the
> change_attribute.

The directory cache_change_attribute is not reported in tracepoints
because it is a directory-specific field, so it's not as useful for
debugging.

The inode change attribute is what we have traditionally used for
determining cache consistency, and when to invalidate the cache.

> 
> Don't we need to send a GETATTR with READDIR for v4?  Not doing so
> means
> that the pagecache is going to behave differently for v3 and v4, and 
> we'll
> potentially end up with totally bogus listings for cases where one 
> reader
> has cached a page of entries in the middle of the pagecache marked
> with
> i_version A, but entries are actually from i_version A++ on the
> server.
> Then another reader comes along and follows earlier entries from 
> i_version A
> on the server that lead into entries from A++.  I don't think we can 
> detect
> this case unless we're checking the directory on every READDIR.

The value of the change attribute is determined when the page is
allocated. That value is unaffected by the READDIR call.

That works with NFSv2 as well as NFSv3+v4.

> 
> Sending a GETATTR for v4 doesn't eliminate that race on the server
> side, 
> but
> does remove the large window on the client created by the attribute 
> cache
> timeouts, and I think its mostly harmless performance-wise.
> 
> Also, we don't need the local change_attr variable just to pass it to
> other
> functions that can access it themselves.
> 
> >         }
> > -
> > +       nfs_readdir_page_init_array(page, last_cookie,
> > change_attr);
> > +       SetPageUptodate(page);
> >         return page;
> >  }
> > 
> > @@ -356,12 +383,6 @@ static void nfs_readdir_page_set_eof(struct
> > page 
> > *page)
> >         kunmap_atomic(array);
> >  }
> > 
> > -static void nfs_readdir_page_unlock_and_put(struct page *page)
> > -{
> > -       unlock_page(page);
> > -       put_page(page);
> > -}
> > -
> >  static struct page *nfs_readdir_page_get_next(struct address_space
> > *mapping,
> >                                               pgoff_t index, u64
> > cookie)
> >  {
> > @@ -418,16 +439,6 @@ static int nfs_readdir_search_for_pos(struct 
> > nfs_cache_array *array,
> >         return -EBADCOOKIE;
> >  }
> > 
> > -static bool
> > -nfs_readdir_inode_mapping_valid(struct nfs_inode *nfsi)
> > -{
> > -       if (nfsi->cache_validity & (NFS_INO_INVALID_CHANGE |
> > -                                   NFS_INO_INVALID_DATA))
> > -               return false;
> > -       smp_rmb();
> > -       return !test_bit(NFS_INO_INVALIDATING, &nfsi->flags);
> > -}
> > -
> >  static bool nfs_readdir_array_cookie_in_range(struct
> > nfs_cache_array 
> > *array,
> >                                               u64 cookie)
> >  {
> > @@ -456,8 +467,7 @@ static int nfs_readdir_search_for_cookie(struct
> > nfs_cache_array *array,
> >                         struct nfs_inode *nfsi =
> > NFS_I(file_inode(desc->file));
> > 
> >                         new_pos = nfs_readdir_page_offset(desc-
> > >page) + i;
> > -                       if (desc->attr_gencount != nfsi-
> > >attr_gencount ||
> > -                           !nfs_readdir_inode_mapping_valid(nfsi))
> > {
> > +                       if (desc->attr_gencount != nfsi-
> > >attr_gencount) {
> >                                 desc->duped = 0;
> >                                 desc->attr_gencount = nfsi-
> > >attr_gencount;
> >                         } else if (new_pos < desc->prev_index) {
> > @@ -1094,11 +1104,7 @@ 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;
> > -       }
> > +       nfs_revalidate_inode(inode, NFS_INO_INVALID_CHANGE);
> 
> Same as above -> why not send GETATTR with READDIR instead of doing
> it 
> in a
> separate RPC?

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