From: Trond Myklebust <[email protected]>
If we're doing uncached readdir(), then the readdir cookie could be
different from the one cached in the nfs_inode. We should therefore
ensure that we save that one in the struct nfs_open_dir_context.
Fixes: 35df59d3ef69 ("NFS: Reduce number of RPC calls when doing uncached readdir")
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 08a1e2e31d0b..2cf2a7d92faf 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -976,10 +976,10 @@ static int readdir_search_pagecache(struct nfs_readdir_descriptor *desc)
/*
* Once we've found the start of the dirent within a page: fill 'er up...
*/
-static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
+static void nfs_do_filldir(struct nfs_readdir_descriptor *desc,
+ const __be32 *verf)
{
struct file *file = desc->file;
- struct nfs_inode *nfsi = NFS_I(file_inode(file));
struct nfs_cache_array *array;
unsigned int i = 0;
@@ -993,7 +993,7 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
desc->eof = true;
break;
}
- memcpy(desc->verf, nfsi->cookieverf, sizeof(desc->verf));
+ memcpy(desc->verf, verf, sizeof(desc->verf));
if (i < (array->size-1))
desc->dir_cookie = array->array[i+1].cookie;
else
@@ -1050,7 +1050,7 @@ static int uncached_readdir(struct nfs_readdir_descriptor *desc)
for (i = 0; !desc->eof && i < sz && arrays[i]; i++) {
desc->page = arrays[i];
- nfs_do_filldir(desc);
+ nfs_do_filldir(desc, verf);
}
desc->page = NULL;
@@ -1071,6 +1071,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
{
struct dentry *dentry = file_dentry(file);
struct inode *inode = d_inode(dentry);
+ struct nfs_inode *nfsi = NFS_I(inode);
struct nfs_open_dir_context *dir_ctx = file->private_data;
struct nfs_readdir_descriptor *desc;
int res;
@@ -1124,7 +1125,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
break;
}
if (res == -ETOOSMALL && desc->plus) {
- clear_bit(NFS_INO_ADVISE_RDPLUS, &NFS_I(inode)->flags);
+ clear_bit(NFS_INO_ADVISE_RDPLUS, &nfsi->flags);
nfs_zap_caches(inode);
desc->page_index = 0;
desc->plus = false;
@@ -1134,7 +1135,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
if (res < 0)
break;
- nfs_do_filldir(desc);
+ nfs_do_filldir(desc, nfsi->cookieverf);
nfs_readdir_page_unlock_and_put_cached(desc);
} while (!desc->eof);
--
2.30.2
Hi Trond,
I think it'll be better to remove cookieverifier from nfs_inode, as truly speaking
it doesn't belong there. A cookie verifier (along with the cookie) is part of a readdir
context, and context is per open dir instance and not per file. Ideally we should have
the following sequence
1. Application opens directory, nfs_open_dir_context->verf is set to 0.
nfs_open_dir_context->verf is used to populate nfs_readdir_descriptor->verf, which is our cursor.
2. The first readdir call using the above newly opened handle carries the 0 cookie verifier (with cookie==0).
3. A cookie verifier received as a response of #2 is saved in nfs_readdir_descriptor->verf, which
is later saved in nfs_open_dir_context->verf. Thus subsequent readdir calls carry the cookie
verifier (and cookie) returned by the server.
What do you think about this patch?
It includes the patch that I sent y'day so only this should be enough.
PS: Currently find_and_lock_cache_page() seems to be accessing nfs_inode->cookieverifier
w/o locking the inode.
Signed-off-by: Nagendra S Tomar <natomar@xxxxxxxxxxxxx>
---
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index fc4f490f2d78..a52917800e37 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -76,6 +76,7 @@ static struct nfs_open_dir_context *alloc_nfs_open_dir_context(struct inode *dir
if (ctx != NULL) {
ctx->duped = 0;
ctx->attr_gencount = nfsi->attr_gencount;
+ memset(ctx->verf, 0, sizeof(ctx->verf));
ctx->dir_cookie = 0;
ctx->dup_cookie = 0;
spin_lock(&dir->i_lock);
@@ -820,7 +821,6 @@ static struct page **nfs_readdir_alloc_pages(size_t npages)
}
static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
- __be32 *verf_arg, __be32 *verf_res,
struct page **arrays, size_t narrays)
{
struct page **pages;
@@ -829,6 +829,7 @@ static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
size_t array_size;
struct inode *inode = file_inode(desc->file);
size_t dtsize = NFS_SERVER(inode)->dtsize;
+ __be32 verf[NFS_DIR_VERIFIER_SIZE];
int status = -ENOMEM;
entry = kzalloc(sizeof(*entry), GFP_KERNEL);
@@ -854,9 +855,9 @@ static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
do {
unsigned int pglen;
- status = nfs_readdir_xdr_filler(desc, verf_arg, entry->cookie,
+ status = nfs_readdir_xdr_filler(desc, desc->verf, entry->cookie,
pages, dtsize,
- verf_res);
+ verf);
if (status < 0)
break;
@@ -866,6 +867,8 @@ static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
break;
}
+ memcpy(desc->verf, verf, sizeof(desc->verf));
+
status = nfs_readdir_page_filler(desc, entry, pages, pglen,
arrays, narrays);
} while (!status && nfs_readdir_page_needs_filling(page));
@@ -909,15 +912,13 @@ static int find_and_lock_cache_page(struct nfs_readdir_descriptor *desc)
{
struct inode *inode = file_inode(desc->file);
struct nfs_inode *nfsi = NFS_I(inode);
- __be32 verf[NFS_DIR_VERIFIER_SIZE];
int res;
desc->page = nfs_readdir_page_get_cached(desc);
if (!desc->page)
return -ENOMEM;
if (nfs_readdir_page_needs_filling(desc->page)) {
- res = nfs_readdir_xdr_to_array(desc, nfsi->cookieverf, verf,
- &desc->page, 1);
+ res = nfs_readdir_xdr_to_array(desc, &desc->page, 1);
if (res < 0) {
nfs_readdir_page_unlock_and_put_cached(desc);
if (res == -EBADCOOKIE || res == -ENOTSYNC) {
@@ -927,7 +928,6 @@ static int find_and_lock_cache_page(struct nfs_readdir_descriptor *desc)
}
return res;
}
- memcpy(nfsi->cookieverf, verf, sizeof(nfsi->cookieverf));
}
res = nfs_readdir_search_array(desc);
if (res == 0) {
@@ -977,7 +977,6 @@ static int readdir_search_pagecache(struct nfs_readdir_descriptor *desc)
static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
{
struct file *file = desc->file;
- struct nfs_inode *nfsi = NFS_I(file_inode(file));
struct nfs_cache_array *array;
unsigned int i = 0;
@@ -991,7 +990,6 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
desc->eof = true;
break;
}
- memcpy(desc->verf, nfsi->cookieverf, sizeof(desc->verf));
if (i < (array->size-1))
desc->dir_cookie = array->array[i+1].cookie;
else
@@ -1027,7 +1025,6 @@ static int uncached_readdir(struct nfs_readdir_descriptor *desc)
{
struct page **arrays;
size_t i, sz = 512;
- __be32 verf[NFS_DIR_VERIFIER_SIZE];
int status = -ENOMEM;
dfprintk(DIRCACHE, "NFS: uncached_readdir() searching for cookie %llu\n",
@@ -1044,7 +1041,7 @@ static int uncached_readdir(struct nfs_readdir_descriptor *desc)
desc->last_cookie = desc->dir_cookie;
desc->duped = 0;
- status = nfs_readdir_xdr_to_array(desc, desc->verf, verf, arrays, sz);
+ status = nfs_readdir_xdr_to_array(desc, arrays, sz);
for (i = 0; !desc->eof && i < sz && arrays[i]; i++) {
desc->page = arrays[i];
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index a7fb076a5f44..ab3e666ed097 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -522,7 +522,6 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr, st
inode->i_uid = make_kuid(&init_user_ns, -2);
inode->i_gid = make_kgid(&init_user_ns, -2);
inode->i_blocks = 0;
- memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf));
nfsi->write_io = 0;
nfsi->read_io = 0;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index eadaabd18dc7..752de9e43e36 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -158,12 +158,6 @@ struct nfs_inode {
struct list_head access_cache_entry_lru;
struct list_head access_cache_inode_lru;
- /*
- * This is the cookie verifier used for NFSv3 readdir
- * operations
- */
- __be32 cookieverf[NFS_DIR_VERIFIER_SIZE];
-
atomic_long_t nrequests;
struct nfs_mds_commit_info commit_info;
-----Original Message-----
From: [email protected] <[email protected]>
Sent: 16 March 2021 19:06
To: Nagendra Tomar <[email protected]>
Cc: [email protected]
Subject: [EXTERNAL] [PATCH] NFS: Fix handling of cookie verifier in uncached_readdir()
From: Trond Myklebust <[email protected]>
If we're doing uncached readdir(), then the readdir cookie could be
different from the one cached in the nfs_inode. We should therefore
ensure that we save that one in the struct nfs_open_dir_context.
Fixes: 35df59d3ef69 ("NFS: Reduce number of RPC calls when doing uncached readdir")
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 08a1e2e31d0b..2cf2a7d92faf 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -976,10 +976,10 @@ static int readdir_search_pagecache(struct nfs_readdir_descriptor *desc)
/*
* Once we've found the start of the dirent within a page: fill 'er up...
*/
-static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
+static void nfs_do_filldir(struct nfs_readdir_descriptor *desc,
+ const __be32 *verf)
{
struct file *file = desc->file;
- struct nfs_inode *nfsi = NFS_I(file_inode(file));
struct nfs_cache_array *array;
unsigned int i = 0;
@@ -993,7 +993,7 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
desc->eof = true;
break;
}
- memcpy(desc->verf, nfsi->cookieverf, sizeof(desc->verf));
+ memcpy(desc->verf, verf, sizeof(desc->verf));
if (i < (array->size-1))
desc->dir_cookie = array->array[i+1].cookie;
else
@@ -1050,7 +1050,7 @@ static int uncached_readdir(struct nfs_readdir_descriptor *desc)
for (i = 0; !desc->eof && i < sz && arrays[i]; i++) {
desc->page = arrays[i];
- nfs_do_filldir(desc);
+ nfs_do_filldir(desc, verf);
}
desc->page = NULL;
@@ -1071,6 +1071,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
{
struct dentry *dentry = file_dentry(file);
struct inode *inode = d_inode(dentry);
+ struct nfs_inode *nfsi = NFS_I(inode);
struct nfs_open_dir_context *dir_ctx = file->private_data;
struct nfs_readdir_descriptor *desc;
int res;
@@ -1124,7 +1125,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
break;
}
if (res == -ETOOSMALL && desc->plus) {
- clear_bit(NFS_INO_ADVISE_RDPLUS, &NFS_I(inode)->flags);
+ clear_bit(NFS_INO_ADVISE_RDPLUS, &nfsi->flags);
nfs_zap_caches(inode);
desc->page_index = 0;
desc->plus = false;
@@ -1134,7 +1135,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
if (res < 0)
break;
- nfs_do_filldir(desc);
+ nfs_do_filldir(desc, nfsi->cookieverf);
nfs_readdir_page_unlock_and_put_cached(desc);
} while (!desc->eof);
--
2.30.2
Ignore this, we do need the inode cookieverifier for the cached cookies in the absence
of any open dir handle. I was clearly overthinking!
I've a question on your patch:
If the server changes the cookieverifier for a directory, shouldn't we zap the
cached directory pages since the cached cookies do not correspond to this new cookie
verifier for this directory? Or, do we depend on the server to return BADCOOKIE when we
present a bad cookieverifier+cookie combo, and then we invalidate. I guess that's fine too.
Also my earlier comment about find_and_lock_cache_page() accessing nfsi->cookieverf
w/o locking the inode. This one is not introduced by your current patch.
Thanks,
Tomar
-----Original Message-----
From: Nagendra Tomar
Sent: 17 March 2021 14:59
To: [email protected]
Cc: [email protected]
Subject: RE: [EXTERNAL] [PATCH] NFS: Fix handling of cookie verifier in uncached_readdir()
Hi Trond,
I think it'll be better to remove cookieverifier from nfs_inode, as truly speaking
it doesn't belong there. A cookie verifier (along with the cookie) is part of a readdir
context, and context is per open dir instance and not per file. Ideally we should have
the following sequence
1. Application opens directory, nfs_open_dir_context->verf is set to 0.
nfs_open_dir_context->verf is used to populate nfs_readdir_descriptor->verf, which is our cursor.
2. The first readdir call using the above newly opened handle carries the 0 cookie verifier (with cookie==0).
3. A cookie verifier received as a response of #2 is saved in nfs_readdir_descriptor->verf, which
is later saved in nfs_open_dir_context->verf. Thus subsequent readdir calls carry the cookie
verifier (and cookie) returned by the server.
What do you think about this patch?
It includes the patch that I sent y'day so only this should be enough.
PS: Currently find_and_lock_cache_page() seems to be accessing nfs_inode->cookieverifier
w/o locking the inode.
Signed-off-by: Nagendra S Tomar <natomar@xxxxxxxxxxxxx>
---
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index fc4f490f2d78..a52917800e37 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -76,6 +76,7 @@ static struct nfs_open_dir_context *alloc_nfs_open_dir_context(struct inode *dir
if (ctx != NULL) {
ctx->duped = 0;
ctx->attr_gencount = nfsi->attr_gencount;
+ memset(ctx->verf, 0, sizeof(ctx->verf));
ctx->dir_cookie = 0;
ctx->dup_cookie = 0;
spin_lock(&dir->i_lock);
@@ -820,7 +821,6 @@ static struct page **nfs_readdir_alloc_pages(size_t npages)
}
static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
- __be32 *verf_arg, __be32 *verf_res,
struct page **arrays, size_t narrays)
{
struct page **pages;
@@ -829,6 +829,7 @@ static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
size_t array_size;
struct inode *inode = file_inode(desc->file);
size_t dtsize = NFS_SERVER(inode)->dtsize;
+ __be32 verf[NFS_DIR_VERIFIER_SIZE];
int status = -ENOMEM;
entry = kzalloc(sizeof(*entry), GFP_KERNEL);
@@ -854,9 +855,9 @@ static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
do {
unsigned int pglen;
- status = nfs_readdir_xdr_filler(desc, verf_arg, entry->cookie,
+ status = nfs_readdir_xdr_filler(desc, desc->verf, entry->cookie,
pages, dtsize,
- verf_res);
+ verf);
if (status < 0)
break;
@@ -866,6 +867,8 @@ static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc,
break;
}
+ memcpy(desc->verf, verf, sizeof(desc->verf));
+
status = nfs_readdir_page_filler(desc, entry, pages, pglen,
arrays, narrays);
} while (!status && nfs_readdir_page_needs_filling(page));
@@ -909,15 +912,13 @@ static int find_and_lock_cache_page(struct nfs_readdir_descriptor *desc)
{
struct inode *inode = file_inode(desc->file);
struct nfs_inode *nfsi = NFS_I(inode);
- __be32 verf[NFS_DIR_VERIFIER_SIZE];
int res;
desc->page = nfs_readdir_page_get_cached(desc);
if (!desc->page)
return -ENOMEM;
if (nfs_readdir_page_needs_filling(desc->page)) {
- res = nfs_readdir_xdr_to_array(desc, nfsi->cookieverf, verf,
- &desc->page, 1);
+ res = nfs_readdir_xdr_to_array(desc, &desc->page, 1);
if (res < 0) {
nfs_readdir_page_unlock_and_put_cached(desc);
if (res == -EBADCOOKIE || res == -ENOTSYNC) {
@@ -927,7 +928,6 @@ static int find_and_lock_cache_page(struct nfs_readdir_descriptor *desc)
}
return res;
}
- memcpy(nfsi->cookieverf, verf, sizeof(nfsi->cookieverf));
}
res = nfs_readdir_search_array(desc);
if (res == 0) {
@@ -977,7 +977,6 @@ static int readdir_search_pagecache(struct nfs_readdir_descriptor *desc)
static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
{
struct file *file = desc->file;
- struct nfs_inode *nfsi = NFS_I(file_inode(file));
struct nfs_cache_array *array;
unsigned int i = 0;
@@ -991,7 +990,6 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
desc->eof = true;
break;
}
- memcpy(desc->verf, nfsi->cookieverf, sizeof(desc->verf));
if (i < (array->size-1))
desc->dir_cookie = array->array[i+1].cookie;
else
@@ -1027,7 +1025,6 @@ static int uncached_readdir(struct nfs_readdir_descriptor *desc)
{
struct page **arrays;
size_t i, sz = 512;
- __be32 verf[NFS_DIR_VERIFIER_SIZE];
int status = -ENOMEM;
dfprintk(DIRCACHE, "NFS: uncached_readdir() searching for cookie %llu\n",
@@ -1044,7 +1041,7 @@ static int uncached_readdir(struct nfs_readdir_descriptor *desc)
desc->last_cookie = desc->dir_cookie;
desc->duped = 0;
- status = nfs_readdir_xdr_to_array(desc, desc->verf, verf, arrays, sz);
+ status = nfs_readdir_xdr_to_array(desc, arrays, sz);
for (i = 0; !desc->eof && i < sz && arrays[i]; i++) {
desc->page = arrays[i];
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index a7fb076a5f44..ab3e666ed097 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -522,7 +522,6 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr, st
inode->i_uid = make_kuid(&init_user_ns, -2);
inode->i_gid = make_kgid(&init_user_ns, -2);
inode->i_blocks = 0;
- memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf));
nfsi->write_io = 0;
nfsi->read_io = 0;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index eadaabd18dc7..752de9e43e36 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -158,12 +158,6 @@ struct nfs_inode {
struct list_head access_cache_entry_lru;
struct list_head access_cache_inode_lru;
- /*
- * This is the cookie verifier used for NFSv3 readdir
- * operations
- */
- __be32 cookieverf[NFS_DIR_VERIFIER_SIZE];
-
atomic_long_t nrequests;
struct nfs_mds_commit_info commit_info;
-----Original Message-----
From: [email protected] <[email protected]>
Sent: 16 March 2021 19:06
To: Nagendra Tomar <[email protected]>
Cc: [email protected]
Subject: [EXTERNAL] [PATCH] NFS: Fix handling of cookie verifier in uncached_readdir()
From: Trond Myklebust <[email protected]>
If we're doing uncached readdir(), then the readdir cookie could be
different from the one cached in the nfs_inode. We should therefore
ensure that we save that one in the struct nfs_open_dir_context.
Fixes: 35df59d3ef69 ("NFS: Reduce number of RPC calls when doing uncached readdir")
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 08a1e2e31d0b..2cf2a7d92faf 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -976,10 +976,10 @@ static int readdir_search_pagecache(struct nfs_readdir_descriptor *desc)
/*
* Once we've found the start of the dirent within a page: fill 'er up...
*/
-static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
+static void nfs_do_filldir(struct nfs_readdir_descriptor *desc,
+ const __be32 *verf)
{
struct file *file = desc->file;
- struct nfs_inode *nfsi = NFS_I(file_inode(file));
struct nfs_cache_array *array;
unsigned int i = 0;
@@ -993,7 +993,7 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
desc->eof = true;
break;
}
- memcpy(desc->verf, nfsi->cookieverf, sizeof(desc->verf));
+ memcpy(desc->verf, verf, sizeof(desc->verf));
if (i < (array->size-1))
desc->dir_cookie = array->array[i+1].cookie;
else
@@ -1050,7 +1050,7 @@ static int uncached_readdir(struct nfs_readdir_descriptor *desc)
for (i = 0; !desc->eof && i < sz && arrays[i]; i++) {
desc->page = arrays[i];
- nfs_do_filldir(desc);
+ nfs_do_filldir(desc, verf);
}
desc->page = NULL;
@@ -1071,6 +1071,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
{
struct dentry *dentry = file_dentry(file);
struct inode *inode = d_inode(dentry);
+ struct nfs_inode *nfsi = NFS_I(inode);
struct nfs_open_dir_context *dir_ctx = file->private_data;
struct nfs_readdir_descriptor *desc;
int res;
@@ -1124,7 +1125,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
break;
}
if (res == -ETOOSMALL && desc->plus) {
- clear_bit(NFS_INO_ADVISE_RDPLUS, &NFS_I(inode)->flags);
+ clear_bit(NFS_INO_ADVISE_RDPLUS, &nfsi->flags);
nfs_zap_caches(inode);
desc->page_index = 0;
desc->plus = false;
@@ -1134,7 +1135,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
if (res < 0)
break;
- nfs_do_filldir(desc);
+ nfs_do_filldir(desc, nfsi->cookieverf);
nfs_readdir_page_unlock_and_put_cached(desc);
} while (!desc->eof);
--
2.30.2
On Wed, 2021-03-17 at 10:39 +0000, Nagendra Tomar wrote:
> Ignore this, we do need the inode cookieverifier for the cached
> cookies in the absence
> of any open dir handle. I was clearly overthinking!
>
Right. When we're caching cookies, then we should also cache the
verifier that is needed to use them. That's why we cache in both the
inode and in the directory context.
> I've a question on your patch:
> If the server changes the cookieverifier for a directory, shouldn't
> we zap the
> cached directory pages since the cached cookies do not correspond to
> this new cookie
> verifier for this directory? Or, do we depend on the server to return
> BADCOOKIE when we
> present a bad cookieverifier+cookie combo, and then we invalidate. I
> guess that's fine too.
>
When the server changes the verifier it will start replying to requests
that use the older verifier with NFS4ERR_NOT_SAME (NFSv4), which we
convert into ENOTSYNC, or NFS3ERR_BAD_COOKIE (NFSv3), which we convert
into EBADCOOKIE.
Both errors are handled in the generic readdir code.
> Also my earlier comment about find_and_lock_cache_page() accessing
> nfsi->cookieverf
> w/o locking the inode. This one is not introduced by your current
> patch.
Yes. I'm aware of that issue. We can just add a check for desc-
>page_index == 0. Nothing should be changing the verifier unless the
cache is empty.
>
> Thanks,
> Tomar
>
> -----Original Message-----
> From: Nagendra Tomar
> Sent: 17 March 2021 14:59
> To: [email protected]
> Cc: [email protected]
> Subject: RE: [EXTERNAL] [PATCH] NFS: Fix handling of cookie verifier
> in uncached_readdir()
>
> Hi Trond,
> I think it'll be better to remove cookieverifier from nfs_inode,
> as truly speaking
> it doesn't belong there. A cookie verifier (along with the cookie) is
> part of a readdir
> context, and context is per open dir instance and not per file.
> Ideally we should have
> the following sequence
>
> 1. Application opens directory, nfs_open_dir_context->verf is set to
> 0.
> nfs_open_dir_context->verf is used to populate
> nfs_readdir_descriptor->verf, which is our cursor.
> 2. The first readdir call using the above newly opened handle carries
> the 0 cookie verifier (with cookie==0).
> 3. A cookie verifier received as a response of #2 is saved in
> nfs_readdir_descriptor->verf, which
> is later saved in nfs_open_dir_context->verf. Thus subsequent
> readdir calls carry the cookie
> verifier (and cookie) returned by the server.
>
> What do you think about this patch?
> It includes the patch that I sent y'day so only this should be
> enough.
>
> PS: Currently find_and_lock_cache_page() seems to be accessing
> nfs_inode->cookieverifier
> w/o locking the inode.
>
> Signed-off-by: Nagendra S Tomar <natomar@xxxxxxxxxxxxx>
> ---
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index fc4f490f2d78..a52917800e37 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -76,6 +76,7 @@ static struct nfs_open_dir_context
> *alloc_nfs_open_dir_context(struct inode *dir
> if (ctx != NULL) {
> ctx->duped = 0;
> ctx->attr_gencount = nfsi->attr_gencount;
> + memset(ctx->verf, 0, sizeof(ctx->verf));
> ctx->dir_cookie = 0;
> ctx->dup_cookie = 0;
> spin_lock(&dir->i_lock);
> @@ -820,7 +821,6 @@ static struct page
> **nfs_readdir_alloc_pages(size_t npages)
> }
>
> static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor
> *desc,
> - __be32 *verf_arg, __be32
> *verf_res,
> struct page **arrays, size_t
> narrays)
> {
> struct page **pages;
> @@ -829,6 +829,7 @@ static int nfs_readdir_xdr_to_array(struct
> nfs_readdir_descriptor *desc,
> size_t array_size;
> struct inode *inode = file_inode(desc->file);
> size_t dtsize = NFS_SERVER(inode)->dtsize;
> + __be32 verf[NFS_DIR_VERIFIER_SIZE];
> int status = -ENOMEM;
>
> entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> @@ -854,9 +855,9 @@ static int nfs_readdir_xdr_to_array(struct
> nfs_readdir_descriptor *desc,
>
> do {
> unsigned int pglen;
> - status = nfs_readdir_xdr_filler(desc, verf_arg,
> entry->cookie,
> + status = nfs_readdir_xdr_filler(desc, desc->verf,
> entry->cookie,
> pages, dtsize,
> - verf_res);
> + verf);
> if (status < 0)
> break;
>
> @@ -866,6 +867,8 @@ static int nfs_readdir_xdr_to_array(struct
> nfs_readdir_descriptor *desc,
> break;
> }
>
> + memcpy(desc->verf, verf, sizeof(desc->verf));
> +
> status = nfs_readdir_page_filler(desc, entry, pages,
> pglen,
> arrays, narrays);
> } while (!status && nfs_readdir_page_needs_filling(page));
> @@ -909,15 +912,13 @@ static int find_and_lock_cache_page(struct
> nfs_readdir_descriptor *desc)
> {
> struct inode *inode = file_inode(desc->file);
> struct nfs_inode *nfsi = NFS_I(inode);
> - __be32 verf[NFS_DIR_VERIFIER_SIZE];
> int res;
>
> desc->page = nfs_readdir_page_get_cached(desc);
> if (!desc->page)
> return -ENOMEM;
> if (nfs_readdir_page_needs_filling(desc->page)) {
> - res = nfs_readdir_xdr_to_array(desc, nfsi-
> >cookieverf, verf,
> - &desc->page, 1);
> + res = nfs_readdir_xdr_to_array(desc, &desc->page, 1);
> if (res < 0) {
> nfs_readdir_page_unlock_and_put_cached(desc);
> if (res == -EBADCOOKIE || res == -ENOTSYNC) {
> @@ -927,7 +928,6 @@ static int find_and_lock_cache_page(struct
> nfs_readdir_descriptor *desc)
> }
> return res;
> }
> - memcpy(nfsi->cookieverf, verf, sizeof(nfsi-
> >cookieverf));
> }
> res = nfs_readdir_search_array(desc);
> if (res == 0) {
> @@ -977,7 +977,6 @@ static int readdir_search_pagecache(struct
> nfs_readdir_descriptor *desc)
> static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
> {
> struct file *file = desc->file;
> - struct nfs_inode *nfsi = NFS_I(file_inode(file));
> struct nfs_cache_array *array;
> unsigned int i = 0;
>
> @@ -991,7 +990,6 @@ static void nfs_do_filldir(struct
> nfs_readdir_descriptor *desc)
> desc->eof = true;
> break;
> }
> - memcpy(desc->verf, nfsi->cookieverf, sizeof(desc-
> >verf));
> if (i < (array->size-1))
> desc->dir_cookie = array->array[i+1].cookie;
> else
> @@ -1027,7 +1025,6 @@ static int uncached_readdir(struct
> nfs_readdir_descriptor *desc)
> {
> struct page **arrays;
> size_t i, sz = 512;
> - __be32 verf[NFS_DIR_VERIFIER_SIZE];
> int status = -ENOMEM;
>
> dfprintk(DIRCACHE, "NFS: uncached_readdir() searching for
> cookie %llu\n",
> @@ -1044,7 +1041,7 @@ static int uncached_readdir(struct
> nfs_readdir_descriptor *desc)
> desc->last_cookie = desc->dir_cookie;
> desc->duped = 0;
>
> - status = nfs_readdir_xdr_to_array(desc, desc->verf, verf,
> arrays, sz);
> + status = nfs_readdir_xdr_to_array(desc, arrays, sz);
>
> for (i = 0; !desc->eof && i < sz && arrays[i]; i++) {
> desc->page = arrays[i];
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index a7fb076a5f44..ab3e666ed097 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -522,7 +522,6 @@ nfs_fhget(struct super_block *sb, struct nfs_fh
> *fh, struct nfs_fattr *fattr, st
> inode->i_uid = make_kuid(&init_user_ns, -2);
> inode->i_gid = make_kgid(&init_user_ns, -2);
> inode->i_blocks = 0;
> - memset(nfsi->cookieverf, 0, sizeof(nfsi-
> >cookieverf));
> nfsi->write_io = 0;
> nfsi->read_io = 0;
>
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index eadaabd18dc7..752de9e43e36 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -158,12 +158,6 @@ struct nfs_inode {
> struct list_head access_cache_entry_lru;
> struct list_head access_cache_inode_lru;
>
> - /*
> - * This is the cookie verifier used for NFSv3 readdir
> - * operations
> - */
> - __be32 cookieverf[NFS_DIR_VERIFIER_SIZE];
> -
> atomic_long_t nrequests;
> struct nfs_mds_commit_info commit_info;
>
>
> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: 16 March 2021 19:06
> To: Nagendra Tomar <[email protected]>
> Cc: [email protected]
> Subject: [EXTERNAL] [PATCH] NFS: Fix handling of cookie verifier in
> uncached_readdir()
>
> From: Trond Myklebust <[email protected]>
>
> If we're doing uncached readdir(), then the readdir cookie could be
> different from the one cached in the nfs_inode. We should therefore
> ensure that we save that one in the struct nfs_open_dir_context.
>
> Fixes: 35df59d3ef69 ("NFS: Reduce number of RPC calls when doing
> uncached readdir")
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/dir.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 08a1e2e31d0b..2cf2a7d92faf 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -976,10 +976,10 @@ static int readdir_search_pagecache(struct
> nfs_readdir_descriptor *desc)
> /*
> * Once we've found the start of the dirent within a page: fill 'er
> up...
> */
> -static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
> +static void nfs_do_filldir(struct nfs_readdir_descriptor *desc,
> + const __be32 *verf)
> {
> struct file *file = desc->file;
> - struct nfs_inode *nfsi = NFS_I(file_inode(file));
> struct nfs_cache_array *array;
> unsigned int i = 0;
>
> @@ -993,7 +993,7 @@ static void nfs_do_filldir(struct
> nfs_readdir_descriptor *desc)
> desc->eof = true;
> break;
> }
> - memcpy(desc->verf, nfsi->cookieverf, sizeof(desc-
> >verf));
> + memcpy(desc->verf, verf, sizeof(desc->verf));
> if (i < (array->size-1))
> desc->dir_cookie = array->array[i+1].cookie;
> else
> @@ -1050,7 +1050,7 @@ static int uncached_readdir(struct
> nfs_readdir_descriptor *desc)
>
> for (i = 0; !desc->eof && i < sz && arrays[i]; i++) {
> desc->page = arrays[i];
> - nfs_do_filldir(desc);
> + nfs_do_filldir(desc, verf);
> }
> desc->page = NULL;
>
> @@ -1071,6 +1071,7 @@ static int nfs_readdir(struct file *file,
> struct dir_context *ctx)
> {
> struct dentry *dentry = file_dentry(file);
> struct inode *inode = d_inode(dentry);
> + struct nfs_inode *nfsi = NFS_I(inode);
> struct nfs_open_dir_context *dir_ctx = file->private_data;
> struct nfs_readdir_descriptor *desc;
> int res;
> @@ -1124,7 +1125,7 @@ static int nfs_readdir(struct file *file,
> struct dir_context *ctx)
> break;
> }
> if (res == -ETOOSMALL && desc->plus) {
> - clear_bit(NFS_INO_ADVISE_RDPLUS,
> &NFS_I(inode)->flags);
> + clear_bit(NFS_INO_ADVISE_RDPLUS, &nfsi-
> >flags);
> nfs_zap_caches(inode);
> desc->page_index = 0;
> desc->plus = false;
> @@ -1134,7 +1135,7 @@ static int nfs_readdir(struct file *file,
> struct dir_context *ctx)
> if (res < 0)
> break;
>
> - nfs_do_filldir(desc);
> + nfs_do_filldir(desc, nfsi->cookieverf);
> nfs_readdir_page_unlock_and_put_cached(desc);
> } while (!desc->eof);
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]