On Tue, 2022-03-01 at 14:09 -0500, Anna Schumaker wrote:
> Hi Trond,
>
> On Mon, Feb 28, 2022 at 5:51 AM <trondmy@xxxxxxxxxx> wrote:
> >
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > Use the change attribute and the first cookie in a directory page
> > cache
> > entry to validate that the page is up to date.
>
> Starting with this patch I'm seeing cthon basic tests fail on NFS v3:
>
> Tue Mar 1 14:08:39 EST 2022
> ./server -b -o tcp,v3,sec=sys -m /mnt/nfsv3tcp -p
> /srv/test/anna/nfsv3tcp server
> ./server -b -o proto=tcp,sec=sys,v4.0 -m /mnt/nfsv4tcp -p
> /srv/test/anna/nfsv4tcp server
> ./server -b -o proto=tcp,sec=sys,v4.1 -m /mnt/nfsv41tcp -p
> /srv/test/anna/nfsv41tcp server
> ./server -b -o proto=tcp,sec=sys,v4.2 -m /mnt/nfsv42tcp -p
> /srv/test/anna/nfsv42tcp server
> Waiting for 'b' to finish...
> The '-b' test using '-o tcp,v3,sec=sys' args to server: Failed!!
> Done: 14:08:41
Which tests are failing, and what is the server configuration that
you're testing against?
I've not been seeing issues with either connectathon or xfstests on the
platforms I've tested.
>
> Anna
> >
> > Suggested-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > ---
> > 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 6f0a38db6c37..bfb553c57274 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -140,6 +140,7 @@ struct nfs_cache_array_entry {
> > };
> >
> > struct nfs_cache_array {
> > + u64 change_attr;
> > u64 last_cookie;
> > unsigned int size;
> > unsigned char page_full : 1,
> > @@ -176,12 +177,14 @@ 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;
> >
> > array = kmap_atomic(page);
> > nfs_readdir_array_init(array);
> > + array->change_attr = change_attr;
> > array->last_cookie = last_cookie;
> > array->cookies_are_ordered = 1;
> > kunmap_atomic(array);
> > @@ -208,7 +211,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;
> > }
> >
> > @@ -305,19 +308,43 @@ int nfs_readdir_add_to_array(struct nfs_entry
> > *entry, struct page *page)
> > return ret;
> > }
> >
> > +static bool nfs_readdir_page_validate(struct page *page, u64
> > last_cookie,
> > + u64 change_attr)
> > +{
> > + struct nfs_cache_array *array = kmap_atomic(page);
> > + int ret = true;
> > +
> > + if (array->change_attr != change_attr)
> > + ret = false;
> > + 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_validate(page, last_cookie,
> > change_attr))
> > + return page;
> > + nfs_readdir_clear_array(page);
> > }
> > -
> > + nfs_readdir_page_init_array(page, last_cookie,
> > change_attr);
> > + SetPageUptodate(page);
> > return page;
> > }
> >
> > @@ -357,12 +384,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)
> > {
> > @@ -419,16 +440,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)
> > {
> > @@ -457,8 +468,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) {
> > @@ -1095,11 +1105,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);
> >
> > res = -ENOMEM;
> > desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> > --
> > 2.35.1
> >
--
Trond Myklebust
CTO, Hammerspace Inc
4984 El Camino Real, Suite 208
Los Altos, CA 94022
www.hammer.space
