From: Trond Myklebust <[email protected]>
Instead of relying on counting the page offsets as we walk through the
page cache, switch to calculating them algorithmically.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 379f88b158fb..6f0a38db6c37 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -249,17 +249,20 @@ static const char *nfs_readdir_copy_name(const char *name, unsigned int len)
return ret;
}
+static size_t nfs_readdir_array_maxentries(void)
+{
+ return (PAGE_SIZE - sizeof(struct nfs_cache_array)) /
+ sizeof(struct nfs_cache_array_entry);
+}
+
/*
* Check that the next array entry lies entirely within the page bounds
*/
static int nfs_readdir_array_can_expand(struct nfs_cache_array *array)
{
- struct nfs_cache_array_entry *cache_entry;
-
if (array->page_full)
return -ENOSPC;
- cache_entry = &array->array[array->size + 1];
- if ((char *)cache_entry - (char *)array > PAGE_SIZE) {
+ if (array->size == nfs_readdir_array_maxentries()) {
array->page_full = 1;
return -ENOSPC;
}
@@ -318,6 +321,11 @@ static struct page *nfs_readdir_page_get_locked(struct address_space *mapping,
return page;
}
+static loff_t nfs_readdir_page_offset(struct page *page)
+{
+ return (loff_t)page->index * (loff_t)nfs_readdir_array_maxentries();
+}
+
static u64 nfs_readdir_page_last_cookie(struct page *page)
{
struct nfs_cache_array *array;
@@ -448,7 +456,7 @@ static int nfs_readdir_search_for_cookie(struct nfs_cache_array *array,
if (array->array[i].cookie == desc->dir_cookie) {
struct nfs_inode *nfsi = NFS_I(file_inode(desc->file));
- new_pos = desc->current_index + i;
+ new_pos = nfs_readdir_page_offset(desc->page) + i;
if (desc->attr_gencount != nfsi->attr_gencount ||
!nfs_readdir_inode_mapping_valid(nfsi)) {
desc->duped = 0;
--
2.35.1
From: Trond Myklebust <[email protected]>
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 <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
---
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
Hi Trond,
On Mon, Feb 28, 2022 at 5:51 AM <[email protected]> wrote:
>
> From: Trond Myklebust <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> 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 <[email protected]> wrote:
> >
> > From: Trond Myklebust <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Trond Myklebust <[email protected]>
> > ---
> > 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
http://www.hammer.space