| Message ID | 20201103153329.531942-17-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Readdir enhancements | expand |
----- Original Message ----- > From: trondmy@kernel.org > To: "linux-nfs" <linux-nfs@vger.kernel.org> > Sent: Tuesday, 3 November, 2020 16:33:29 > Subject: [PATCH v2 16/16] NFS: Improve handling of directory verifiers > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > If the server insists on using the readdir verifiers in order to allow > cookies to expire, then we should ensure that we cache the verifier > with the cookie, so that we can return an error if the application > tries to use the expired cookie. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfs/dir.c | 35 +++++++++++++++++++++++------------ > fs/nfs/inode.c | 7 ------- > include/linux/nfs_fs.h | 8 +++++++- > 3 files changed, 30 insertions(+), 20 deletions(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index 1c5a5f9cb228..0bd9cc625bdb 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -155,6 +155,7 @@ struct nfs_readdir_descriptor { > loff_t current_index; > loff_t prev_index; > > + __be32 verf[NFS_DIR_VERIFIER_SIZE]; > unsigned long dir_verifier; > unsigned long timestamp; > unsigned long gencount; > @@ -467,15 +468,15 @@ static int nfs_readdir_search_array(struct > nfs_readdir_descriptor *desc) > > /* Fill a page with xdr information before transferring to the cache page */ > static int nfs_readdir_xdr_filler(struct nfs_readdir_descriptor *desc, > - u64 cookie, struct page **pages, > - size_t bufsize) > + __be32 *verf, u64 cookie, > + struct page **pages, size_t bufsize, > + __be32 *verf_res) > { > struct inode *inode = file_inode(desc->file); > - __be32 verf_res[2]; > struct nfs_readdir_arg arg = { > .dentry = file_dentry(desc->file), > .cred = desc->file->f_cred, > - .verf = NFS_I(inode)->cookieverf, > + .verf = verf, > .cookie = cookie, > .pages = pages, > .page_len = bufsize, > @@ -504,8 +505,6 @@ static int nfs_readdir_xdr_filler(struct > nfs_readdir_descriptor *desc, > } > desc->timestamp = timestamp; > desc->gencount = gencount; > - memcpy(NFS_I(inode)->cookieverf, res.verf, > - sizeof(NFS_I(inode)->cookieverf)); > error: > return error; > } > @@ -771,11 +770,13 @@ static struct page **nfs_readdir_alloc_pages(size_t > npages) > } > > static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc, > - struct page *page, struct inode *inode) > + struct page *page, __be32 *verf_arg, > + __be32 *verf_res) > { > struct page **pages; > struct nfs_entry *entry; > size_t array_size; > + struct inode *inode = file_inode(desc->file); > size_t dtsize = NFS_SERVER(inode)->dtsize; > int status = -ENOMEM; > > @@ -802,8 +803,9 @@ static int nfs_readdir_xdr_to_array(struct > nfs_readdir_descriptor *desc, > > do { > unsigned int pglen; > - status = nfs_readdir_xdr_filler(desc, entry->cookie, > - pages, dtsize); > + status = nfs_readdir_xdr_filler(desc, verf_arg, entry->cookie, > + pages, dtsize, > + verf_res); > if (status < 0) > break; > > @@ -855,13 +857,15 @@ 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, desc->page, inode); > + res = nfs_readdir_xdr_to_array(desc, desc->page, > + nfsi->cookieverf, verf); > if (res < 0) { > nfs_readdir_page_unlock_and_put_cached(desc); > if (res == -EBADCOOKIE || res == -ENOTSYNC) { > @@ -871,6 +875,7 @@ 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) { > @@ -903,6 +908,7 @@ 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; > > @@ -916,6 +922,7 @@ 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 > @@ -950,8 +957,8 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor > *desc) > static int uncached_readdir(struct nfs_readdir_descriptor *desc) > { > struct page *page = NULL; > + __be32 verf[NFS_DIR_VERIFIER_SIZE]; > int status; > - struct inode *inode = file_inode(desc->file); > > dfprintk(DIRCACHE, "NFS: uncached_readdir() searching for cookie %Lu\n", > (unsigned long long)desc->dir_cookie); > @@ -968,7 +975,7 @@ static int uncached_readdir(struct nfs_readdir_descriptor > *desc) > desc->duped = 0; > > nfs_readdir_page_init_array(page, desc->dir_cookie); > - status = nfs_readdir_xdr_to_array(desc, page, inode); > + status = nfs_readdir_xdr_to_array(desc, page, desc->verf, verf); > if (status < 0) > goto out_release; > > @@ -1024,6 +1031,7 @@ static int nfs_readdir(struct file *file, struct > dir_context *ctx) > desc->dup_cookie = dir_ctx->dup_cookie; > desc->duped = dir_ctx->duped; > desc->attr_gencount = dir_ctx->attr_gencount; > + memcpy(desc->verf, dir_ctx->verf, sizeof(desc->verf)); > spin_unlock(&file->f_lock); > > do { > @@ -1062,6 +1070,7 @@ static int nfs_readdir(struct file *file, struct > dir_context *ctx) > dir_ctx->dup_cookie = desc->dup_cookie; > dir_ctx->duped = desc->duped; > dir_ctx->attr_gencount = desc->attr_gencount; > + memcpy(dir_ctx->verf, desc->verf, sizeof(dir_ctx->verf)); > spin_unlock(&file->f_lock); > > kfree(desc); > @@ -1102,6 +1111,8 @@ static loff_t nfs_llseek_dir(struct file *filp, loff_t > offset, int whence) > dir_ctx->dir_cookie = offset; > else > dir_ctx->dir_cookie = 0; > + if (offset == 0) > + memset(dir_ctx->verf, 0, sizeof(dir_ctx->verf)); > dir_ctx->duped = 0; > } > spin_unlock(&filp->f_lock); > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c > index aa6493905bbe..9b765a900b28 100644 > --- a/fs/nfs/inode.c > +++ b/fs/nfs/inode.c > @@ -229,7 +229,6 @@ static void nfs_zap_caches_locked(struct inode *inode) > nfsi->attrtimeo = NFS_MINATTRTIMEO(inode); > nfsi->attrtimeo_timestamp = jiffies; > > - memset(NFS_I(inode)->cookieverf, 0, sizeof(NFS_I(inode)->cookieverf)); > if (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)) { > nfs_set_cache_invalid(inode, NFS_INO_INVALID_ATTR > | NFS_INO_INVALID_DATA > @@ -1237,7 +1236,6 @@ EXPORT_SYMBOL_GPL(nfs_revalidate_inode); > > static int nfs_invalidate_mapping(struct inode *inode, struct address_space > *mapping) > { > - struct nfs_inode *nfsi = NFS_I(inode); > int ret; > > if (mapping->nrpages != 0) { > @@ -1250,11 +1248,6 @@ static int nfs_invalidate_mapping(struct inode *inode, > struct address_space *map > if (ret < 0) > return ret; > } > - if (S_ISDIR(inode->i_mode)) { > - spin_lock(&inode->i_lock); > - memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf)); > - spin_unlock(&inode->i_lock); > - } > nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE); > nfs_fscache_wait_on_invalidate(inode); > > diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h > index dd6b463dda80..681ed98e4ba8 100644 > --- a/include/linux/nfs_fs.h > +++ b/include/linux/nfs_fs.h > @@ -45,6 +45,11 @@ > */ > #define NFS_RPC_SWAPFLAGS (RPC_TASK_SWAPPER|RPC_TASK_ROOTCREDS) > > +/* > + * Size of the NFS directory verifier > + */ > +#define NFS_DIR_VERIFIER_SIZE 2 > + > /* > * NFSv3/v4 Access mode cache entry > */ > @@ -89,6 +94,7 @@ struct nfs_open_context { > struct nfs_open_dir_context { > struct list_head list; > unsigned long attr_gencount; > + __be32 verf[NFS_DIR_VERIFIER_SIZE]; > __u64 dir_cookie; > __u64 dup_cookie; > signed char duped; > @@ -156,7 +162,7 @@ struct nfs_inode { > * This is the cookie verifier used for NFSv3 readdir > * operations > */ > - __be32 cookieverf[2]; > + __be32 cookieverf[NFS_DIR_VERIFIER_SIZE]; Just for my education. Why we use 2x32 bit BE encoded ints instead of raw 8 bytes? And if it's treaded as a number, as spec sometimes does ("The request's cookieverf field should be set to 0"), then why it's not then __be64? Tigran. > > atomic_long_t nrequests; > struct nfs_mds_commit_info commit_info; > -- > 2.28.0
Hi Tigran, On Wed, 2020-11-04 at 11:12 +0100, Mkrtchyan, Tigran wrote: > > > ----- Original Message ----- > > From: trondmy@kernel.org > > To: "linux-nfs" <linux-nfs@vger.kernel.org> > > Sent: Tuesday, 3 November, 2020 16:33:29 > > Subject: [PATCH v2 16/16] NFS: Improve handling of directory > > verifiers > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> <snip> > > @@ -89,6 +94,7 @@ struct nfs_open_context { > > struct nfs_open_dir_context { > > struct list_head list; > > unsigned long attr_gencount; > > + __be32 verf[NFS_DIR_VERIFIER_SIZE]; > > __u64 dir_cookie; > > __u64 dup_cookie; > > signed char duped; > > @@ -156,7 +162,7 @@ struct nfs_inode { > > * This is the cookie verifier used for NFSv3 readdir > > * operations > > */ > > - __be32 cookieverf[2]; > > + __be32 cookieverf[NFS_DIR_VERIFIER_SIZE]; > > Just for my education. Why we use 2x32 bit BE encoded ints instead of > raw 8 bytes? > And if it's treaded as a number, as spec sometimes does ("The > request's cookieverf > field should be set to 0"), then why it's not then __be64? The main reason for often using __be32 on these identifiers is to tag them as being opaque RPC objects (since RPC objects are always in units of 32-bits and are big endian encoded).
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 1c5a5f9cb228..0bd9cc625bdb 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -155,6 +155,7 @@ struct nfs_readdir_descriptor { loff_t current_index; loff_t prev_index; + __be32 verf[NFS_DIR_VERIFIER_SIZE]; unsigned long dir_verifier; unsigned long timestamp; unsigned long gencount; @@ -467,15 +468,15 @@ static int nfs_readdir_search_array(struct nfs_readdir_descriptor *desc) /* Fill a page with xdr information before transferring to the cache page */ static int nfs_readdir_xdr_filler(struct nfs_readdir_descriptor *desc, - u64 cookie, struct page **pages, - size_t bufsize) + __be32 *verf, u64 cookie, + struct page **pages, size_t bufsize, + __be32 *verf_res) { struct inode *inode = file_inode(desc->file); - __be32 verf_res[2]; struct nfs_readdir_arg arg = { .dentry = file_dentry(desc->file), .cred = desc->file->f_cred, - .verf = NFS_I(inode)->cookieverf, + .verf = verf, .cookie = cookie, .pages = pages, .page_len = bufsize, @@ -504,8 +505,6 @@ static int nfs_readdir_xdr_filler(struct nfs_readdir_descriptor *desc, } desc->timestamp = timestamp; desc->gencount = gencount; - memcpy(NFS_I(inode)->cookieverf, res.verf, - sizeof(NFS_I(inode)->cookieverf)); error: return error; } @@ -771,11 +770,13 @@ static struct page **nfs_readdir_alloc_pages(size_t npages) } static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc, - struct page *page, struct inode *inode) + struct page *page, __be32 *verf_arg, + __be32 *verf_res) { struct page **pages; struct nfs_entry *entry; size_t array_size; + struct inode *inode = file_inode(desc->file); size_t dtsize = NFS_SERVER(inode)->dtsize; int status = -ENOMEM; @@ -802,8 +803,9 @@ static int nfs_readdir_xdr_to_array(struct nfs_readdir_descriptor *desc, do { unsigned int pglen; - status = nfs_readdir_xdr_filler(desc, entry->cookie, - pages, dtsize); + status = nfs_readdir_xdr_filler(desc, verf_arg, entry->cookie, + pages, dtsize, + verf_res); if (status < 0) break; @@ -855,13 +857,15 @@ 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, desc->page, inode); + res = nfs_readdir_xdr_to_array(desc, desc->page, + nfsi->cookieverf, verf); if (res < 0) { nfs_readdir_page_unlock_and_put_cached(desc); if (res == -EBADCOOKIE || res == -ENOTSYNC) { @@ -871,6 +875,7 @@ 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) { @@ -903,6 +908,7 @@ 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; @@ -916,6 +922,7 @@ 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 @@ -950,8 +957,8 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc) static int uncached_readdir(struct nfs_readdir_descriptor *desc) { struct page *page = NULL; + __be32 verf[NFS_DIR_VERIFIER_SIZE]; int status; - struct inode *inode = file_inode(desc->file); dfprintk(DIRCACHE, "NFS: uncached_readdir() searching for cookie %Lu\n", (unsigned long long)desc->dir_cookie); @@ -968,7 +975,7 @@ static int uncached_readdir(struct nfs_readdir_descriptor *desc) desc->duped = 0; nfs_readdir_page_init_array(page, desc->dir_cookie); - status = nfs_readdir_xdr_to_array(desc, page, inode); + status = nfs_readdir_xdr_to_array(desc, page, desc->verf, verf); if (status < 0) goto out_release; @@ -1024,6 +1031,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx) desc->dup_cookie = dir_ctx->dup_cookie; desc->duped = dir_ctx->duped; desc->attr_gencount = dir_ctx->attr_gencount; + memcpy(desc->verf, dir_ctx->verf, sizeof(desc->verf)); spin_unlock(&file->f_lock); do { @@ -1062,6 +1070,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx) dir_ctx->dup_cookie = desc->dup_cookie; dir_ctx->duped = desc->duped; dir_ctx->attr_gencount = desc->attr_gencount; + memcpy(dir_ctx->verf, desc->verf, sizeof(dir_ctx->verf)); spin_unlock(&file->f_lock); kfree(desc); @@ -1102,6 +1111,8 @@ static loff_t nfs_llseek_dir(struct file *filp, loff_t offset, int whence) dir_ctx->dir_cookie = offset; else dir_ctx->dir_cookie = 0; + if (offset == 0) + memset(dir_ctx->verf, 0, sizeof(dir_ctx->verf)); dir_ctx->duped = 0; } spin_unlock(&filp->f_lock); diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index aa6493905bbe..9b765a900b28 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -229,7 +229,6 @@ static void nfs_zap_caches_locked(struct inode *inode) nfsi->attrtimeo = NFS_MINATTRTIMEO(inode); nfsi->attrtimeo_timestamp = jiffies; - memset(NFS_I(inode)->cookieverf, 0, sizeof(NFS_I(inode)->cookieverf)); if (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)) { nfs_set_cache_invalid(inode, NFS_INO_INVALID_ATTR | NFS_INO_INVALID_DATA @@ -1237,7 +1236,6 @@ EXPORT_SYMBOL_GPL(nfs_revalidate_inode); static int nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping) { - struct nfs_inode *nfsi = NFS_I(inode); int ret; if (mapping->nrpages != 0) { @@ -1250,11 +1248,6 @@ static int nfs_invalidate_mapping(struct inode *inode, struct address_space *map if (ret < 0) return ret; } - if (S_ISDIR(inode->i_mode)) { - spin_lock(&inode->i_lock); - memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf)); - spin_unlock(&inode->i_lock); - } nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE); nfs_fscache_wait_on_invalidate(inode); diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index dd6b463dda80..681ed98e4ba8 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -45,6 +45,11 @@ */ #define NFS_RPC_SWAPFLAGS (RPC_TASK_SWAPPER|RPC_TASK_ROOTCREDS) +/* + * Size of the NFS directory verifier + */ +#define NFS_DIR_VERIFIER_SIZE 2 + /* * NFSv3/v4 Access mode cache entry */ @@ -89,6 +94,7 @@ struct nfs_open_context { struct nfs_open_dir_context { struct list_head list; unsigned long attr_gencount; + __be32 verf[NFS_DIR_VERIFIER_SIZE]; __u64 dir_cookie; __u64 dup_cookie; signed char duped; @@ -156,7 +162,7 @@ struct nfs_inode { * This is the cookie verifier used for NFSv3 readdir * operations */ - __be32 cookieverf[2]; + __be32 cookieverf[NFS_DIR_VERIFIER_SIZE]; atomic_long_t nrequests; struct nfs_mds_commit_info commit_info;