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
