| Message ID | 20220227231227.9038-8-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Readdir improvements | expand |
Hi Trond, On Mon, Feb 28, 2022 at 5:51 AM <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. 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 Anna > > 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 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 >
On Tue, 2022-03-01 at 14:09 -0500, Anna Schumaker wrote: > Hi Trond, > > On Mon, Feb 28, 2022 at 5:51 AM <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. > > 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@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 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 > >
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);