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: trondmy@xxxxxxxxxx
Cc: linux-nfs@xxxxxxxxxxxxxxx
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: trondmy@xxxxxxxxxx <trondmy@xxxxxxxxxx>
Sent: 16 March 2021 19:06
To: Nagendra Tomar <Nagendra.Tomar@xxxxxxxxxxxxx>
Cc: linux-nfs@xxxxxxxxxxxxxxx
Subject: [EXTERNAL] [PATCH] NFS: Fix handling of cookie verifier in uncached_readdir()
From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
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 <trond.myklebust@xxxxxxxxxxxxxxx>
---
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
