* [PATCH 0/3] Include OFD lock owners when looking up state
@ 2016-04-01 15:34 Benjamin Coddington
2016-04-01 15:34 ` [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context Benjamin Coddington
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Benjamin Coddington @ 2016-04-01 15:34 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Jeff Layton; +Cc: linux-nfs
The client sends IO only under the open stateid when using OFD (and flock)
locking instead of the appropriate lock stateid because the nfs_lock_context
only tracks POSIX lockowners, which is the reference to the process' file
table.
This is a problem for two reasons. The first is that rfc7530,
section-9.1.4.5 states that IO sent by an entity corresponding to the
lock-owner which holds a byte-range lock should be sent under the lock
stateid for that lock. Otherwise, a server enforcing mandatory byte-range
locking might reject that operation. Secondly, not tracking OFD lock owners
means that accounting for IO sent under those owners is broken. That
creates a problem for some work to guarantee an unlock will be sent after
operations scheduled under a lock complete.
To fix this, this set starts tracking the OFD lock owner within the
nfs_lock_context. Unique nfs_lock_contexts are created on distinct
combinations of current->files, struct file, and pid. In order to do this,
some re-arrangement of when to create or lookup the nfs_lock_context was
needed since the struct file pointer must be included during that creation
or lookup.
It is possible for a process to hold both an OFD and a POSIX lock on a file
as long as they do not conflict. There would be two stateids that could be
selected for IO operations on that file. In that case only the first
stateid found to match the current lock context will ever be used. The
result of this is that mixed OFD and POSIX locks within the same process on
a server enforcing mandatory locking should be avoided. The fix for this
probably would require a byte-range lookup of stateid when scheduling an IO
operation, and the ability to split IO that crossed lock ranges with
different owners.
Comments and critique is welcomed.
Benjamin Coddington (3):
NFS: add get_nfs_lock_context, find_nfs_lock_context
NFS: add OFD lock owners to nfs_lock_context
NFSv4: use OFD lock owners in lock state lookup
fs/nfs/direct.c | 8 ++++----
fs/nfs/file.c | 2 +-
fs/nfs/fscache-index.c | 4 ++--
fs/nfs/fscache.h | 12 ++++++------
fs/nfs/inode.c | 32 ++++++++++++++++++++++----------
fs/nfs/nfs42proc.c | 8 ++++----
fs/nfs/nfs4proc.c | 3 ++-
fs/nfs/nfs4state.c | 21 ++++++++++++---------
fs/nfs/pagelist.c | 22 ++++++++--------------
fs/nfs/read.c | 38 +++++++++++++++++++++++---------------
fs/nfs/write.c | 17 ++++++++++-------
include/linux/nfs_fs.h | 8 +++++---
include/linux/nfs_page.h | 2 +-
13 files changed, 100 insertions(+), 77 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context 2016-04-01 15:34 [PATCH 0/3] Include OFD lock owners when looking up state Benjamin Coddington @ 2016-04-01 15:34 ` Benjamin Coddington 2016-04-01 16:45 ` kbuild test robot 2016-04-01 17:03 ` kbuild test robot 2016-04-01 15:34 ` [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context Benjamin Coddington ` (3 subsequent siblings) 4 siblings, 2 replies; 16+ messages in thread From: Benjamin Coddington @ 2016-04-01 15:34 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker, Jeff Layton; +Cc: linux-nfs Create a method for acquiring a nfs_lock_context reference which will be used later within fscache, and rename nfs_get_lock_context to nfs_find_lock_context to prepare it for finding or allocating a lock context based upon a file pointer. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/direct.c | 4 ++-- fs/nfs/file.c | 2 +- fs/nfs/inode.c | 11 +++++++++-- fs/nfs/nfs42proc.c | 8 ++++---- fs/nfs/pagelist.c | 2 +- include/linux/nfs_fs.h | 3 ++- 6 files changed, 19 insertions(+), 11 deletions(-) diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c index 7a0cfd3..5f833fa 100644 --- a/fs/nfs/direct.c +++ b/fs/nfs/direct.c @@ -596,7 +596,7 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, struct iov_iter *iter, dreq->bytes_left = count; dreq->io_start = pos; dreq->ctx = get_nfs_open_context(nfs_file_open_context(iocb->ki_filp)); - l_ctx = nfs_get_lock_context(dreq->ctx); + l_ctx = nfs_find_lock_context(dreq->ctx); if (IS_ERR(l_ctx)) { result = PTR_ERR(l_ctx); goto out_release; @@ -1029,7 +1029,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter) dreq->bytes_left = iov_iter_count(iter); dreq->io_start = pos; dreq->ctx = get_nfs_open_context(nfs_file_open_context(iocb->ki_filp)); - l_ctx = nfs_get_lock_context(dreq->ctx); + l_ctx = nfs_find_lock_context(dreq->ctx); if (IS_ERR(l_ctx)) { result = PTR_ERR(l_ctx); goto out_release; diff --git a/fs/nfs/file.c b/fs/nfs/file.c index 748bb81..f99c085 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -754,7 +754,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local) */ vfs_fsync(filp, 0); - l_ctx = nfs_get_lock_context(nfs_file_open_context(filp)); + l_ctx = nfs_find_lock_context(nfs_file_open_context(filp)); if (!IS_ERR(l_ctx)) { status = nfs_iocounter_wait(l_ctx); nfs_put_lock_context(l_ctx); diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 86faecf..5d484e5 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -725,7 +725,7 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context return NULL; } -struct nfs_lock_context *nfs_get_lock_context(struct nfs_open_context *ctx) +struct nfs_lock_context *nfs_find_lock_context(struct nfs_open_context *ctx) { struct nfs_lock_context *res, *new = NULL; struct inode *inode = d_inode(ctx->dentry); @@ -751,7 +751,14 @@ struct nfs_lock_context *nfs_get_lock_context(struct nfs_open_context *ctx) kfree(new); return res; } -EXPORT_SYMBOL_GPL(nfs_get_lock_context); +EXPORT_SYMBOL_GPL(nfs_find_lock_context); + +struct nfs_lock_context *get_nfs_lock_context(struct nfs_lock_context *l_ctx) +{ + atomic_inc(&l_ctx->count); + return l_ctx; +} +EXPORT_SYMBOL_GPL(get_nfs_lock_context); void nfs_put_lock_context(struct nfs_lock_context *l_ctx) { diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c index dff8346..f9b0f25 100644 --- a/fs/nfs/nfs42proc.c +++ b/fs/nfs/nfs42proc.c @@ -61,7 +61,7 @@ static int nfs42_proc_fallocate(struct rpc_message *msg, struct file *filep, struct nfs_lock_context *lock; int err; - lock = nfs_get_lock_context(nfs_file_open_context(filep)); + lock = nfs_find_lock_context(nfs_file_open_context(filep)); if (IS_ERR(lock)) return PTR_ERR(lock); @@ -171,7 +171,7 @@ loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int whence) struct nfs_lock_context *lock; loff_t err; - lock = nfs_get_lock_context(nfs_file_open_context(filep)); + lock = nfs_find_lock_context(nfs_file_open_context(filep)); if (IS_ERR(lock)) return PTR_ERR(lock); @@ -365,14 +365,14 @@ int nfs42_proc_clone(struct file *src_f, struct file *dst_f, if (!nfs_server_capable(inode, NFS_CAP_CLONE)) return -EOPNOTSUPP; - src_lock = nfs_get_lock_context(nfs_file_open_context(src_f)); + src_lock = nfs_find_lock_context(nfs_file_open_context(src_f)); if (IS_ERR(src_lock)) return PTR_ERR(src_lock); src_exception.inode = file_inode(src_f); src_exception.state = src_lock->open_context->state; - dst_lock = nfs_get_lock_context(nfs_file_open_context(dst_f)); + dst_lock = nfs_find_lock_context(nfs_file_open_context(dst_f)); if (IS_ERR(dst_lock)) { err = PTR_ERR(dst_lock); goto out_put_src_lock; diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c index 8ce4f61..b3e5366 100644 --- a/fs/nfs/pagelist.c +++ b/fs/nfs/pagelist.c @@ -329,7 +329,7 @@ nfs_create_request(struct nfs_open_context *ctx, struct page *page, return ERR_PTR(-ENOMEM); /* get lock context early so we can deal with alloc failures */ - l_ctx = nfs_get_lock_context(ctx); + l_ctx = nfs_find_lock_context(ctx); if (IS_ERR(l_ctx)) { nfs_page_free(req); return ERR_CAST(l_ctx); diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 67300f8..e93e285 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -365,7 +365,8 @@ extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fm extern void nfs_inode_attach_open_context(struct nfs_open_context *ctx); extern void nfs_file_set_open_context(struct file *filp, struct nfs_open_context *ctx); extern void nfs_file_clear_open_context(struct file *flip); -extern struct nfs_lock_context *nfs_get_lock_context(struct nfs_open_context *ctx); +extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); +extern struct nfs_lock_context *get_nfs_lock_context(struct nfs_lock_context *l_ctx); extern void nfs_put_lock_context(struct nfs_lock_context *l_ctx); extern u64 nfs_compat_user_ino64(u64 fileid); extern void nfs_fattr_init(struct nfs_fattr *fattr); -- 1.7.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context 2016-04-01 15:34 ` [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context Benjamin Coddington @ 2016-04-01 16:45 ` kbuild test robot 2016-04-01 17:03 ` kbuild test robot 1 sibling, 0 replies; 16+ messages in thread From: kbuild test robot @ 2016-04-01 16:45 UTC (permalink / raw) To: Benjamin Coddington Cc: kbuild-all, Trond Myklebust, Anna Schumaker, Jeff Layton, linux-nfs [-- Attachment #1: Type: text/plain, Size: 3802 bytes --] Hi Benjamin, [auto build test ERROR on v4.6-rc1] [also build test ERROR on next-20160401] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Benjamin-Coddington/Include-OFD-lock-owners-when-looking-up-state/20160401-233801 config: mips-jz4740 (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=mips Note: the linux-review/Benjamin-Coddington/Include-OFD-lock-owners-when-looking-up-state/20160401-233801 HEAD e6973cc5e04cb6af43dbba362969dbf6ddcf7740 builds fine. It only hurts bisectibility. All errors (new ones prefixed by >>): fs/nfs/file.c: In function 'do_unlk': >> fs/nfs/file.c:759:32: error: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type [-Werror=incompatible-pointer-types] l_ctx = nfs_find_lock_context(nfs_file_open_context(filp)); ^ In file included from fs/nfs/file.c:25:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ cc1: some warnings being treated as errors -- fs/nfs/direct.c: In function 'nfs_file_direct_read': >> fs/nfs/direct.c:599:32: error: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type [-Werror=incompatible-pointer-types] l_ctx = nfs_find_lock_context(dreq->ctx); ^ In file included from fs/nfs/direct.c:51:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ fs/nfs/direct.c: In function 'nfs_file_direct_write': fs/nfs/direct.c:1032:32: error: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type [-Werror=incompatible-pointer-types] l_ctx = nfs_find_lock_context(dreq->ctx); ^ In file included from fs/nfs/direct.c:51:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ cc1: some warnings being treated as errors -- fs/nfs/pagelist.c: In function 'nfs_create_request': >> fs/nfs/pagelist.c:332:32: error: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type [-Werror=incompatible-pointer-types] l_ctx = nfs_find_lock_context(ctx); ^ In file included from fs/nfs/pagelist.c:20:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ cc1: some warnings being treated as errors vim +/nfs_find_lock_context +759 fs/nfs/file.c 753 /* 754 * Flush all pending writes before doing anything 755 * with locks.. 756 */ 757 vfs_fsync(filp, 0); 758 > 759 l_ctx = nfs_find_lock_context(nfs_file_open_context(filp)); 760 if (!IS_ERR(l_ctx)) { 761 status = nfs_iocounter_wait(l_ctx); 762 nfs_put_lock_context(l_ctx); --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/octet-stream, Size: 18246 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context 2016-04-01 15:34 ` [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context Benjamin Coddington 2016-04-01 16:45 ` kbuild test robot @ 2016-04-01 17:03 ` kbuild test robot 1 sibling, 0 replies; 16+ messages in thread From: kbuild test robot @ 2016-04-01 17:03 UTC (permalink / raw) To: Benjamin Coddington Cc: kbuild-all, Trond Myklebust, Anna Schumaker, Jeff Layton, linux-nfs [-- Attachment #1: Type: text/plain, Size: 7787 bytes --] Hi Benjamin, [auto build test ERROR on v4.6-rc1] [also build test ERROR on next-20160401] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Benjamin-Coddington/Include-OFD-lock-owners-when-looking-up-state/20160401-233801 config: sparc64-allyesconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=sparc64 Note: the linux-review/Benjamin-Coddington/Include-OFD-lock-owners-when-looking-up-state/20160401-233801 HEAD e6973cc5e04cb6af43dbba362969dbf6ddcf7740 builds fine. It only hurts bisectibility. All error/warnings (new ones prefixed by >>): fs/nfs/file.c: In function 'do_unlk': >> fs/nfs/file.c:759:10: warning: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type l_ctx = nfs_find_lock_context(nfs_file_open_context(filp)); ^ In file included from fs/nfs/file.c:25:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ -- >> fs/nfs/inode.c:728:26: error: conflicting types for 'nfs_find_lock_context' struct nfs_lock_context *nfs_find_lock_context(struct nfs_open_context *ctx) ^ In file included from fs/nfs/inode.c:29:0: include/linux/nfs_fs.h:368:33: note: previous declaration of 'nfs_find_lock_context' was here extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ In file included from include/linux/linkage.h:6:0, from include/linux/kernel.h:6, from include/linux/list.h:8, from include/linux/module.h:9, from fs/nfs/inode.c:16: fs/nfs/inode.c:754:19: error: conflicting types for 'nfs_find_lock_context' EXPORT_SYMBOL_GPL(nfs_find_lock_context); ^ include/linux/export.h:57:21: note: in definition of macro '__EXPORT_SYMBOL' extern typeof(sym) sym; \ ^ >> fs/nfs/inode.c:754:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL' EXPORT_SYMBOL_GPL(nfs_find_lock_context); ^ In file included from fs/nfs/inode.c:29:0: include/linux/nfs_fs.h:368:33: note: previous declaration of 'nfs_find_lock_context' was here extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ -- fs/nfs/direct.c: In function 'nfs_file_direct_read': >> fs/nfs/direct.c:599:10: warning: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type l_ctx = nfs_find_lock_context(dreq->ctx); ^ In file included from fs/nfs/direct.c:51:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ fs/nfs/direct.c: In function 'nfs_file_direct_write': fs/nfs/direct.c:1032:10: warning: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type l_ctx = nfs_find_lock_context(dreq->ctx); ^ In file included from fs/nfs/direct.c:51:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ -- fs/nfs/pagelist.c: In function 'nfs_create_request': >> fs/nfs/pagelist.c:332:10: warning: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type l_ctx = nfs_find_lock_context(ctx); ^ In file included from fs/nfs/pagelist.c:20:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ -- fs/nfs/nfs42proc.c: In function 'nfs42_proc_fallocate': >> fs/nfs/nfs42proc.c:64:9: warning: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type lock = nfs_find_lock_context(nfs_file_open_context(filep)); ^ In file included from fs/nfs/nfs42proc.c:10:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ fs/nfs/nfs42proc.c: In function 'nfs42_proc_llseek': fs/nfs/nfs42proc.c:174:9: warning: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type lock = nfs_find_lock_context(nfs_file_open_context(filep)); ^ In file included from fs/nfs/nfs42proc.c:10:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ fs/nfs/nfs42proc.c: In function 'nfs42_proc_clone': fs/nfs/nfs42proc.c:368:13: warning: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type src_lock = nfs_find_lock_context(nfs_file_open_context(src_f)); ^ In file included from fs/nfs/nfs42proc.c:10:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ fs/nfs/nfs42proc.c:375:13: warning: passing argument 1 of 'nfs_find_lock_context' from incompatible pointer type dst_lock = nfs_find_lock_context(nfs_file_open_context(dst_f)); ^ In file included from fs/nfs/nfs42proc.c:10:0: include/linux/nfs_fs.h:368:33: note: expected 'struct file *' but argument is of type 'struct nfs_open_context *' extern struct nfs_lock_context *nfs_find_lock_context(struct file *file); ^ vim +/nfs_find_lock_context +728 fs/nfs/inode.c 722 atomic_inc(&pos->count); 723 return pos; 724 } while ((pos = list_entry(pos->list.next, typeof(*pos), list)) != head); 725 return NULL; 726 } 727 > 728 struct nfs_lock_context *nfs_find_lock_context(struct nfs_open_context *ctx) 729 { 730 struct nfs_lock_context *res, *new = NULL; 731 struct inode *inode = d_inode(ctx->dentry); 732 733 spin_lock(&inode->i_lock); 734 res = __nfs_find_lock_context(ctx); 735 if (res == NULL) { 736 spin_unlock(&inode->i_lock); 737 new = kmalloc(sizeof(*new), GFP_KERNEL); 738 if (new == NULL) 739 return ERR_PTR(-ENOMEM); 740 nfs_init_lock_context(new); 741 spin_lock(&inode->i_lock); 742 res = __nfs_find_lock_context(ctx); 743 if (res == NULL) { 744 list_add_tail(&new->list, &ctx->lock_context.list); 745 new->open_context = ctx; 746 res = new; 747 new = NULL; 748 } 749 } 750 spin_unlock(&inode->i_lock); 751 kfree(new); 752 return res; 753 } > 754 EXPORT_SYMBOL_GPL(nfs_find_lock_context); 755 756 struct nfs_lock_context *get_nfs_lock_context(struct nfs_lock_context *l_ctx) 757 { --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/octet-stream, Size: 45936 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context 2016-04-01 15:34 [PATCH 0/3] Include OFD lock owners when looking up state Benjamin Coddington 2016-04-01 15:34 ` [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context Benjamin Coddington @ 2016-04-01 15:34 ` Benjamin Coddington 2016-04-01 16:17 ` kbuild test robot 2016-04-01 16:55 ` kbuild test robot 2016-04-01 15:34 ` [PATCH 3/3] NFSv4: use OFD lock owners in lock state lookup Benjamin Coddington ` (2 subsequent siblings) 4 siblings, 2 replies; 16+ messages in thread From: Benjamin Coddington @ 2016-04-01 15:34 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker, Jeff Layton; +Cc: linux-nfs OFD locks use the file pointer for lock ownership. Change lock context lookup to take a file pointer which will create distinct lock contexts for operations on separate files. This allows us to retain the OFD lock owner so we can find a matching nfs4_lock_state later. IO paths are modified to lookup or create the lock context earlier in the call path where we have a reference to a file pointer. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/direct.c | 8 ++++---- fs/nfs/file.c | 2 +- fs/nfs/fscache-index.c | 4 ++-- fs/nfs/fscache.h | 12 ++++++------ fs/nfs/inode.c | 23 ++++++++++++++--------- fs/nfs/nfs42proc.c | 8 ++++---- fs/nfs/nfs4proc.c | 3 ++- fs/nfs/nfs4state.c | 2 +- fs/nfs/pagelist.c | 21 +++++++-------------- fs/nfs/read.c | 38 +++++++++++++++++++++++--------------- fs/nfs/write.c | 16 +++++++++------- include/linux/nfs_fs.h | 5 +++-- include/linux/nfs_page.h | 2 +- 13 files changed, 77 insertions(+), 67 deletions(-) diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c index 5f833fa..8f49e95 100644 --- a/fs/nfs/direct.c +++ b/fs/nfs/direct.c @@ -499,7 +499,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq, struct nfs_page *req; unsigned int req_len = min_t(size_t, bytes, PAGE_SIZE - pgbase); /* XXX do we need to do the eof zeroing found in async_filler? */ - req = nfs_create_request(dreq->ctx, pagevec[i], NULL, + req = nfs_create_request(dreq->l_ctx, pagevec[i], NULL, pgbase, req_len); if (IS_ERR(req)) { result = PTR_ERR(req); @@ -596,7 +596,7 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, struct iov_iter *iter, dreq->bytes_left = count; dreq->io_start = pos; dreq->ctx = get_nfs_open_context(nfs_file_open_context(iocb->ki_filp)); - l_ctx = nfs_find_lock_context(dreq->ctx); + l_ctx = nfs_find_lock_context(file); if (IS_ERR(l_ctx)) { result = PTR_ERR(l_ctx); goto out_release; @@ -915,7 +915,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq, struct nfs_page *req; unsigned int req_len = min_t(size_t, bytes, PAGE_SIZE - pgbase); - req = nfs_create_request(dreq->ctx, pagevec[i], NULL, + req = nfs_create_request(dreq->l_ctx, pagevec[i], NULL, pgbase, req_len); if (IS_ERR(req)) { result = PTR_ERR(req); @@ -1029,7 +1029,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter) dreq->bytes_left = iov_iter_count(iter); dreq->io_start = pos; dreq->ctx = get_nfs_open_context(nfs_file_open_context(iocb->ki_filp)); - l_ctx = nfs_find_lock_context(dreq->ctx); + l_ctx = nfs_find_lock_context(file); if (IS_ERR(l_ctx)) { result = PTR_ERR(l_ctx); goto out_release; diff --git a/fs/nfs/file.c b/fs/nfs/file.c index f99c085..c1361c6 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -754,7 +754,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local) */ vfs_fsync(filp, 0); - l_ctx = nfs_find_lock_context(nfs_file_open_context(filp)); + l_ctx = nfs_find_lock_context(filp); if (!IS_ERR(l_ctx)) { status = nfs_iocounter_wait(l_ctx); nfs_put_lock_context(l_ctx); diff --git a/fs/nfs/fscache-index.c b/fs/nfs/fscache-index.c index 777b055..c182d57 100644 --- a/fs/nfs/fscache-index.c +++ b/fs/nfs/fscache-index.c @@ -300,7 +300,7 @@ static void nfs_fscache_inode_now_uncached(void *cookie_netfs_data) */ static void nfs_fh_get_context(void *cookie_netfs_data, void *context) { - get_nfs_open_context(context); + get_nfs_lock_context(context); } /* @@ -311,7 +311,7 @@ static void nfs_fh_get_context(void *cookie_netfs_data, void *context) static void nfs_fh_put_context(void *cookie_netfs_data, void *context) { if (context) - put_nfs_open_context(context); + put_nfs_lock_context(context); } /* diff --git a/fs/nfs/fscache.h b/fs/nfs/fscache.h index d7fe3e7..5e08751 100644 --- a/fs/nfs/fscache.h +++ b/fs/nfs/fscache.h @@ -114,26 +114,26 @@ static inline void nfs_fscache_invalidate_page(struct page *page, /* * Retrieve a page from an inode data storage object. */ -static inline int nfs_readpage_from_fscache(struct nfs_open_context *ctx, +static inline int nfs_readpage_from_fscache(struct nfs_lock_context *l_ctx, struct inode *inode, struct page *page) { if (NFS_I(inode)->fscache) - return __nfs_readpage_from_fscache(ctx, inode, page); + return __nfs_readpage_from_fscache(l_ctx, inode, page); return -ENOBUFS; } /* * Retrieve a set of pages from an inode data storage object. */ -static inline int nfs_readpages_from_fscache(struct nfs_open_context *ctx, +static inline int nfs_readpages_from_fscache(struct nfs_lock_context *l_ctx, struct inode *inode, struct address_space *mapping, struct list_head *pages, unsigned *nr_pages) { if (NFS_I(inode)->fscache) - return __nfs_readpages_from_fscache(ctx, inode, mapping, pages, + return __nfs_readpages_from_fscache(l_ctx, inode, mapping, pages, nr_pages); return -ENOBUFS; } @@ -199,13 +199,13 @@ static inline void nfs_fscache_invalidate_page(struct page *page, static inline void nfs_fscache_wait_on_page_write(struct nfs_inode *nfsi, struct page *page) {} -static inline int nfs_readpage_from_fscache(struct nfs_open_context *ctx, +static inline int nfs_readpage_from_fscache(struct nfs_lock_context *l_ctx, struct inode *inode, struct page *page) { return -ENOBUFS; } -static inline int nfs_readpages_from_fscache(struct nfs_open_context *ctx, +static inline int nfs_readpages_from_fscache(struct nfs_lock_context *l_ctx, struct inode *inode, struct address_space *mapping, struct list_head *pages, diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 5d484e5..a703a02 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -700,22 +700,25 @@ out: } EXPORT_SYMBOL_GPL(nfs_getattr); -static void nfs_init_lock_context(struct nfs_lock_context *l_ctx) +static void nfs_init_lock_context(struct nfs_lock_context *l_ctx, struct file *file) { atomic_set(&l_ctx->count, 1); - l_ctx->lockowner.l_owner = current->files; + l_ctx->lockowner.l_owner_posix = current->files; + l_ctx->lockowner.l_owner_ofd = file; l_ctx->lockowner.l_pid = current->tgid; INIT_LIST_HEAD(&l_ctx->list); atomic_set(&l_ctx->io_count, 0); } -static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context *ctx) +static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context *ctx, struct file *file) { struct nfs_lock_context *head = &ctx->lock_context; struct nfs_lock_context *pos = head; do { - if (pos->lockowner.l_owner != current->files) + if (pos->lockowner.l_owner_posix != current->files) + continue; + if (pos->lockowner.l_owner_ofd != file) continue; if (pos->lockowner.l_pid != current->tgid) continue; @@ -725,21 +728,22 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context return NULL; } -struct nfs_lock_context *nfs_find_lock_context(struct nfs_open_context *ctx) +struct nfs_lock_context *nfs_find_lock_context(struct file *file) { + struct nfs_open_context *ctx = nfs_file_open_context(file); struct nfs_lock_context *res, *new = NULL; struct inode *inode = d_inode(ctx->dentry); spin_lock(&inode->i_lock); - res = __nfs_find_lock_context(ctx); + res = __nfs_find_lock_context(ctx, file); if (res == NULL) { spin_unlock(&inode->i_lock); new = kmalloc(sizeof(*new), GFP_KERNEL); if (new == NULL) return ERR_PTR(-ENOMEM); - nfs_init_lock_context(new); + nfs_init_lock_context(new, file); spin_lock(&inode->i_lock); - res = __nfs_find_lock_context(ctx); + res = __nfs_find_lock_context(ctx, file); if (res == NULL) { list_add_tail(&new->list, &ctx->lock_context.list); new->open_context = ctx; @@ -826,7 +830,7 @@ struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f ctx->mode = f_mode; ctx->flags = 0; ctx->error = 0; - nfs_init_lock_context(&ctx->lock_context); + nfs_init_lock_context(&ctx->lock_context, NULL); ctx->lock_context.open_context = ctx; INIT_LIST_HEAD(&ctx->list); ctx->mdsthreshold = NULL; @@ -893,6 +897,7 @@ EXPORT_SYMBOL_GPL(nfs_inode_attach_open_context); void nfs_file_set_open_context(struct file *filp, struct nfs_open_context *ctx) { filp->private_data = get_nfs_open_context(ctx); + ctx->lock_context.lockowner.l_owner_ofd = filp; if (list_empty(&ctx->list)) nfs_inode_attach_open_context(ctx); } diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c index f9b0f25..107a182 100644 --- a/fs/nfs/nfs42proc.c +++ b/fs/nfs/nfs42proc.c @@ -61,7 +61,7 @@ static int nfs42_proc_fallocate(struct rpc_message *msg, struct file *filep, struct nfs_lock_context *lock; int err; - lock = nfs_find_lock_context(nfs_file_open_context(filep)); + lock = nfs_find_lock_context(filep); if (IS_ERR(lock)) return PTR_ERR(lock); @@ -171,7 +171,7 @@ loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int whence) struct nfs_lock_context *lock; loff_t err; - lock = nfs_find_lock_context(nfs_file_open_context(filep)); + lock = nfs_find_lock_context(filep); if (IS_ERR(lock)) return PTR_ERR(lock); @@ -365,14 +365,14 @@ int nfs42_proc_clone(struct file *src_f, struct file *dst_f, if (!nfs_server_capable(inode, NFS_CAP_CLONE)) return -EOPNOTSUPP; - src_lock = nfs_find_lock_context(nfs_file_open_context(src_f)); + src_lock = nfs_find_lock_context(src_f); if (IS_ERR(src_lock)) return PTR_ERR(src_lock); src_exception.inode = file_inode(src_f); src_exception.state = src_lock->open_context->state; - dst_lock = nfs_find_lock_context(nfs_file_open_context(dst_f)); + dst_lock = nfs_find_lock_context(dst_f); if (IS_ERR(dst_lock)) { err = PTR_ERR(dst_lock); goto out_put_src_lock; diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 1488159..cb5b326 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -2694,7 +2694,8 @@ static int _nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred, /* Use that stateid */ } else if (truncate && state != NULL) { struct nfs_lockowner lockowner = { - .l_owner = current->files, + .l_owner_posix = current->files, + .l_owner_ofd = sattr->ia_file, .l_pid = current->tgid, }; if (!nfs4_valid_open_stateid(state)) diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index d854693..b7f4509 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -952,7 +952,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst, if (test_bit(LK_STATE_IN_USE, &state->flags) == 0) goto out; - fl_owner = lockowner->l_owner; + fl_owner = lockowner->l_owner_posix; spin_lock(&state->state_lock); lsp = __nfs4_find_lock_state(state, fl_owner); if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags)) diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c index b3e5366..2b28dff 100644 --- a/fs/nfs/pagelist.c +++ b/fs/nfs/pagelist.c @@ -314,27 +314,20 @@ nfs_page_group_destroy(struct kref *kref) * User should ensure it is safe to sleep in this function. */ struct nfs_page * -nfs_create_request(struct nfs_open_context *ctx, struct page *page, +nfs_create_request(struct nfs_lock_context *l_ctx, struct page *page, struct nfs_page *last, unsigned int offset, unsigned int count) { struct nfs_page *req; - struct nfs_lock_context *l_ctx; - if (test_bit(NFS_CONTEXT_BAD, &ctx->flags)) + if (test_bit(NFS_CONTEXT_BAD, &l_ctx->open_context->flags)) return ERR_PTR(-EBADF); /* try to allocate the request struct */ req = nfs_page_alloc(); if (req == NULL) return ERR_PTR(-ENOMEM); - /* get lock context early so we can deal with alloc failures */ - l_ctx = nfs_find_lock_context(ctx); - if (IS_ERR(l_ctx)) { - nfs_page_free(req); - return ERR_CAST(l_ctx); - } - req->wb_lock_context = l_ctx; + req->wb_lock_context = get_nfs_lock_context(l_ctx); atomic_inc(&l_ctx->io_count); /* Initialize the request struct. Initially, we assume a @@ -346,7 +339,7 @@ nfs_create_request(struct nfs_open_context *ctx, struct page *page, req->wb_offset = offset; req->wb_pgbase = offset; req->wb_bytes = count; - req->wb_context = get_nfs_open_context(ctx); + req->wb_context = get_nfs_open_context(l_ctx->open_context); kref_init(&req->wb_kref); nfs_page_group_init(req, last); return req; @@ -865,7 +858,7 @@ static void nfs_pageio_cleanup_mirroring(struct nfs_pageio_descriptor *pgio) static bool nfs_match_lock_context(const struct nfs_lock_context *l1, const struct nfs_lock_context *l2) { - return l1->lockowner.l_owner == l2->lockowner.l_owner + return l1->lockowner.l_owner_posix == l2->lockowner.l_owner_posix && l1->lockowner.l_pid == l2->lockowner.l_pid; } @@ -1024,7 +1017,7 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc, pgbase += subreq->wb_bytes; if (bytes_left) { - subreq = nfs_create_request(req->wb_context, + subreq = nfs_create_request(req->wb_lock_context, req->wb_page, subreq, pgbase, bytes_left); if (IS_ERR(subreq)) @@ -1115,7 +1108,7 @@ int nfs_pageio_add_request(struct nfs_pageio_descriptor *desc, lastreq = lastreq->wb_this_page) ; - dupreq = nfs_create_request(req->wb_context, + dupreq = nfs_create_request(req->wb_lock_context, req->wb_page, lastreq, pgbase, bytes); if (IS_ERR(dupreq)) { diff --git a/fs/nfs/read.c b/fs/nfs/read.c index eb31e23..704effd 100644 --- a/fs/nfs/read.c +++ b/fs/nfs/read.c @@ -102,7 +102,7 @@ static void nfs_readpage_release(struct nfs_page *req) nfs_release_request(req); } -int nfs_readpage_async(struct nfs_open_context *ctx, struct inode *inode, +int nfs_readpage_async(struct nfs_lock_context *l_ctx, struct inode *inode, struct page *page) { struct nfs_page *new; @@ -113,7 +113,7 @@ int nfs_readpage_async(struct nfs_open_context *ctx, struct inode *inode, len = nfs_page_length(page); if (len == 0) return nfs_return_empty_page(page); - new = nfs_create_request(ctx, page, NULL, 0, len); + new = nfs_create_request(l_ctx, page, NULL, 0, len); if (IS_ERR(new)) { unlock_page(page); return PTR_ERR(new); @@ -291,6 +291,7 @@ static void nfs_readpage_result(struct rpc_task *task, int nfs_readpage(struct file *file, struct page *page) { struct nfs_open_context *ctx; + struct nfs_lock_context *l_ctx; struct inode *inode = page_file_mapping(page)->host; int error; @@ -321,19 +322,22 @@ int nfs_readpage(struct file *file, struct page *page) ctx = nfs_find_open_context(inode, NULL, FMODE_READ); if (ctx == NULL) goto out_unlock; - } else - ctx = get_nfs_open_context(nfs_file_open_context(file)); + l_ctx = get_nfs_lock_context(&ctx->lock_context); + put_nfs_open_context(ctx); + } else { + l_ctx = nfs_find_lock_context(file); + } if (!IS_SYNC(inode)) { - error = nfs_readpage_from_fscache(ctx, inode, page); + error = nfs_readpage_from_fscache(l_ctx, inode, page); if (error == 0) goto out; } - error = nfs_readpage_async(ctx, inode, page); + error = nfs_readpage_async(l_ctx, inode, page); out: - put_nfs_open_context(ctx); + nfs_put_lock_context(l_ctx); return error; out_unlock: unlock_page(page); @@ -342,7 +346,7 @@ out_unlock: struct nfs_readdesc { struct nfs_pageio_descriptor *pgio; - struct nfs_open_context *ctx; + struct nfs_lock_context *l_ctx; }; static int @@ -357,7 +361,7 @@ readpage_async_filler(void *data, struct page *page) if (len == 0) return nfs_return_empty_page(page); - new = nfs_create_request(desc->ctx, page, NULL, 0, len); + new = nfs_create_request(desc->l_ctx, page, NULL, 0, len); if (IS_ERR(new)) goto out_error; @@ -382,6 +386,7 @@ int nfs_readpages(struct file *filp, struct address_space *mapping, { struct nfs_pageio_descriptor pgio; struct nfs_pgio_mirror *pgm; + struct nfs_open_context *ctx; struct nfs_readdesc desc = { .pgio = &pgio, }; @@ -399,16 +404,19 @@ int nfs_readpages(struct file *filp, struct address_space *mapping, goto out; if (filp == NULL) { - desc.ctx = nfs_find_open_context(inode, NULL, FMODE_READ); - if (desc.ctx == NULL) + ctx = nfs_find_open_context(inode, NULL, FMODE_READ); + if (ctx == NULL) return -EBADF; - } else - desc.ctx = get_nfs_open_context(nfs_file_open_context(filp)); + desc.l_ctx = get_nfs_lock_context(&ctx->lock_context); + put_nfs_open_context(ctx); + } else { + desc.l_ctx = nfs_find_lock_context(filp); + } /* attempt to read as many of the pages as possible from the cache * - this returns -ENOBUFS immediately if the cookie is negative */ - ret = nfs_readpages_from_fscache(desc.ctx, inode, mapping, + ret = nfs_readpages_from_fscache(desc.l_ctx, inode, mapping, pages, &nr_pages); if (ret == 0) goto read_complete; /* all pages were read */ @@ -428,7 +436,7 @@ int nfs_readpages(struct file *filp, struct address_space *mapping, PAGE_CACHE_SHIFT; nfs_add_stats(inode, NFSIOS_READPAGES, npages); read_complete: - put_nfs_open_context(desc.ctx); + nfs_put_lock_context(desc.l_ctx); out: return ret; } diff --git a/fs/nfs/write.c b/fs/nfs/write.c index 5754835..935244c 100644 --- a/fs/nfs/write.c +++ b/fs/nfs/write.c @@ -1101,7 +1101,7 @@ out_err: * if we have to add a new request. Also assumes that the caller has * already called nfs_flush_incompatible() if necessary. */ -static struct nfs_page * nfs_setup_write_request(struct nfs_open_context* ctx, +static struct nfs_page * nfs_setup_write_request(struct nfs_lock_context *l_ctx, struct page *page, unsigned int offset, unsigned int bytes) { struct inode *inode = page_file_mapping(page)->host; @@ -1110,7 +1110,7 @@ static struct nfs_page * nfs_setup_write_request(struct nfs_open_context* ctx, req = nfs_try_to_update_request(inode, page, offset, bytes); if (req != NULL) goto out; - req = nfs_create_request(ctx, page, NULL, offset, bytes); + req = nfs_create_request(l_ctx, page, NULL, offset, bytes); if (IS_ERR(req)) goto out; nfs_inode_add_request(inode, req); @@ -1118,12 +1118,12 @@ out: return req; } -static int nfs_writepage_setup(struct nfs_open_context *ctx, struct page *page, +static int nfs_writepage_setup(struct nfs_lock_context *l_ctx, struct page *page, unsigned int offset, unsigned int count) { struct nfs_page *req; - req = nfs_setup_write_request(ctx, page, offset, count); + req = nfs_setup_write_request(l_ctx, page, offset, count); if (IS_ERR(req)) return PTR_ERR(req); /* Update file length */ @@ -1161,7 +1161,7 @@ int nfs_flush_incompatible(struct file *file, struct page *page) if (l_ctx && flctx && !(list_empty_careful(&flctx->flc_posix) && list_empty_careful(&flctx->flc_flock))) { - do_flush |= l_ctx->lockowner.l_owner != current->files + do_flush |= l_ctx->lockowner.l_owner_posix != current->files || l_ctx->lockowner.l_pid != current->tgid; } nfs_release_request(req); @@ -1279,7 +1279,7 @@ static int nfs_can_extend_write(struct file *file, struct page *page, struct ino int nfs_updatepage(struct file *file, struct page *page, unsigned int offset, unsigned int count) { - struct nfs_open_context *ctx = nfs_file_open_context(file); + struct nfs_lock_context *l_ctx = nfs_find_lock_context(file); struct inode *inode = page_file_mapping(page)->host; int status = 0; @@ -1293,12 +1293,14 @@ int nfs_updatepage(struct file *file, struct page *page, offset = 0; } - status = nfs_writepage_setup(ctx, page, offset, count); + status = nfs_writepage_setup(l_ctx, page, offset, count); if (status < 0) nfs_set_pageerror(page); else __set_page_dirty_nobuffers(page); + nfs_put_lock_context(l_ctx); + dprintk("NFS: nfs_updatepage returns %d (isize %lld)\n", status, (long long)i_size_read(inode)); return status; diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index e93e285..cf43e23 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -56,7 +56,8 @@ struct nfs_access_entry { }; struct nfs_lockowner { - fl_owner_t l_owner; + fl_owner_t l_owner_posix; + fl_owner_t l_owner_ofd; pid_t l_pid; }; @@ -542,7 +543,7 @@ nfs_have_writebacks(struct inode *inode) extern int nfs_readpage(struct file *, struct page *); extern int nfs_readpages(struct file *, struct address_space *, struct list_head *, unsigned); -extern int nfs_readpage_async(struct nfs_open_context *, struct inode *, +extern int nfs_readpage_async(struct nfs_lock_context *, struct inode *, struct page *); /* diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h index f2f650f..3b0cbcd 100644 --- a/include/linux/nfs_page.h +++ b/include/linux/nfs_page.h @@ -110,7 +110,7 @@ struct nfs_pageio_descriptor { #define NFS_WBACK_BUSY(req) (test_bit(PG_BUSY,&(req)->wb_flags)) -extern struct nfs_page *nfs_create_request(struct nfs_open_context *ctx, +extern struct nfs_page *nfs_create_request(struct nfs_lock_context *l_ctx, struct page *page, struct nfs_page *last, unsigned int offset, -- 1.7.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context 2016-04-01 15:34 ` [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context Benjamin Coddington @ 2016-04-01 16:17 ` kbuild test robot 2016-04-01 16:55 ` kbuild test robot 1 sibling, 0 replies; 16+ messages in thread From: kbuild test robot @ 2016-04-01 16:17 UTC (permalink / raw) To: Benjamin Coddington Cc: kbuild-all, Trond Myklebust, Anna Schumaker, Jeff Layton, linux-nfs [-- Attachment #1: Type: text/plain, Size: 3477 bytes --] Hi Benjamin, [auto build test ERROR on v4.6-rc1] [also build test ERROR on next-20160401] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Benjamin-Coddington/Include-OFD-lock-owners-when-looking-up-state/20160401-233801 config: sparc64-allmodconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=sparc64 All error/warnings (new ones prefixed by >>): In file included from fs/nfs/nfs4file.c:14:0: fs/nfs/fscache.h: In function 'nfs_readpage_from_fscache': >> fs/nfs/fscache.h:122:10: warning: passing argument 1 of '__nfs_readpage_from_fscache' from incompatible pointer type return __nfs_readpage_from_fscache(l_ctx, inode, page); ^ fs/nfs/fscache.h:86:12: note: expected 'struct nfs_open_context *' but argument is of type 'struct nfs_lock_context *' extern int __nfs_readpage_from_fscache(struct nfs_open_context *, ^ fs/nfs/fscache.h: In function 'nfs_readpages_from_fscache': >> fs/nfs/fscache.h:136:10: warning: passing argument 1 of '__nfs_readpages_from_fscache' from incompatible pointer type return __nfs_readpages_from_fscache(l_ctx, inode, mapping, pages, ^ fs/nfs/fscache.h:88:12: note: expected 'struct nfs_open_context *' but argument is of type 'struct nfs_lock_context *' extern int __nfs_readpages_from_fscache(struct nfs_open_context *, ^ -- In file included from fs/nfs/fscache-index.c:21:0: fs/nfs/fscache.h: In function 'nfs_readpage_from_fscache': >> fs/nfs/fscache.h:122:10: warning: passing argument 1 of '__nfs_readpage_from_fscache' from incompatible pointer type return __nfs_readpage_from_fscache(l_ctx, inode, page); ^ fs/nfs/fscache.h:86:12: note: expected 'struct nfs_open_context *' but argument is of type 'struct nfs_lock_context *' extern int __nfs_readpage_from_fscache(struct nfs_open_context *, ^ fs/nfs/fscache.h: In function 'nfs_readpages_from_fscache': >> fs/nfs/fscache.h:136:10: warning: passing argument 1 of '__nfs_readpages_from_fscache' from incompatible pointer type return __nfs_readpages_from_fscache(l_ctx, inode, mapping, pages, ^ fs/nfs/fscache.h:88:12: note: expected 'struct nfs_open_context *' but argument is of type 'struct nfs_lock_context *' extern int __nfs_readpages_from_fscache(struct nfs_open_context *, ^ fs/nfs/fscache-index.c: In function 'nfs_fh_put_context': >> fs/nfs/fscache-index.c:314:3: error: implicit declaration of function 'put_nfs_lock_context' [-Werror=implicit-function-declaration] put_nfs_lock_context(context); ^ cc1: some warnings being treated as errors vim +/put_nfs_lock_context +314 fs/nfs/fscache-index.c 308 * - This function can be absent if the completion function doesn't require a 309 * context. 310 */ 311 static void nfs_fh_put_context(void *cookie_netfs_data, void *context) 312 { 313 if (context) > 314 put_nfs_lock_context(context); 315 } 316 317 /* --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/octet-stream, Size: 45793 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context 2016-04-01 15:34 ` [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context Benjamin Coddington 2016-04-01 16:17 ` kbuild test robot @ 2016-04-01 16:55 ` kbuild test robot 1 sibling, 0 replies; 16+ messages in thread From: kbuild test robot @ 2016-04-01 16:55 UTC (permalink / raw) To: Benjamin Coddington Cc: kbuild-all, Trond Myklebust, Anna Schumaker, Jeff Layton, linux-nfs [-- Attachment #1: Type: text/plain, Size: 4156 bytes --] Hi Benjamin, [auto build test ERROR on v4.6-rc1] [also build test ERROR on next-20160401] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Benjamin-Coddington/Include-OFD-lock-owners-when-looking-up-state/20160401-233801 config: i386-randconfig-s0-201613 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): In file included from fs/nfs/inode.c:49:0: fs/nfs/fscache.h: In function 'nfs_readpage_from_fscache': >> fs/nfs/fscache.h:122:38: error: passing argument 1 of '__nfs_readpage_from_fscache' from incompatible pointer type [-Werror=incompatible-pointer-types] return __nfs_readpage_from_fscache(l_ctx, inode, page); ^ fs/nfs/fscache.h:86:12: note: expected 'struct nfs_open_context *' but argument is of type 'struct nfs_lock_context *' extern int __nfs_readpage_from_fscache(struct nfs_open_context *, ^ fs/nfs/fscache.h: In function 'nfs_readpages_from_fscache': >> fs/nfs/fscache.h:136:39: error: passing argument 1 of '__nfs_readpages_from_fscache' from incompatible pointer type [-Werror=incompatible-pointer-types] return __nfs_readpages_from_fscache(l_ctx, inode, mapping, pages, ^ fs/nfs/fscache.h:88:12: note: expected 'struct nfs_open_context *' but argument is of type 'struct nfs_lock_context *' extern int __nfs_readpages_from_fscache(struct nfs_open_context *, ^ cc1: some warnings being treated as errors -- In file included from fs/nfs/fscache-index.c:21:0: fs/nfs/fscache.h: In function 'nfs_readpage_from_fscache': >> fs/nfs/fscache.h:122:38: error: passing argument 1 of '__nfs_readpage_from_fscache' from incompatible pointer type [-Werror=incompatible-pointer-types] return __nfs_readpage_from_fscache(l_ctx, inode, page); ^ fs/nfs/fscache.h:86:12: note: expected 'struct nfs_open_context *' but argument is of type 'struct nfs_lock_context *' extern int __nfs_readpage_from_fscache(struct nfs_open_context *, ^ fs/nfs/fscache.h: In function 'nfs_readpages_from_fscache': >> fs/nfs/fscache.h:136:39: error: passing argument 1 of '__nfs_readpages_from_fscache' from incompatible pointer type [-Werror=incompatible-pointer-types] return __nfs_readpages_from_fscache(l_ctx, inode, mapping, pages, ^ fs/nfs/fscache.h:88:12: note: expected 'struct nfs_open_context *' but argument is of type 'struct nfs_lock_context *' extern int __nfs_readpages_from_fscache(struct nfs_open_context *, ^ fs/nfs/fscache-index.c: In function 'nfs_fh_put_context': fs/nfs/fscache-index.c:314:3: error: implicit declaration of function 'put_nfs_lock_context' [-Werror=implicit-function-declaration] put_nfs_lock_context(context); ^ cc1: some warnings being treated as errors vim +/__nfs_readpage_from_fscache +122 fs/nfs/fscache.h 116 */ 117 static inline int nfs_readpage_from_fscache(struct nfs_lock_context *l_ctx, 118 struct inode *inode, 119 struct page *page) 120 { 121 if (NFS_I(inode)->fscache) > 122 return __nfs_readpage_from_fscache(l_ctx, inode, page); 123 return -ENOBUFS; 124 } 125 126 /* 127 * Retrieve a set of pages from an inode data storage object. 128 */ 129 static inline int nfs_readpages_from_fscache(struct nfs_lock_context *l_ctx, 130 struct inode *inode, 131 struct address_space *mapping, 132 struct list_head *pages, 133 unsigned *nr_pages) 134 { 135 if (NFS_I(inode)->fscache) > 136 return __nfs_readpages_from_fscache(l_ctx, inode, mapping, pages, 137 nr_pages); 138 return -ENOBUFS; 139 } --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/octet-stream, Size: 29360 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] NFSv4: use OFD lock owners in lock state lookup 2016-04-01 15:34 [PATCH 0/3] Include OFD lock owners when looking up state Benjamin Coddington 2016-04-01 15:34 ` [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context Benjamin Coddington 2016-04-01 15:34 ` [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context Benjamin Coddington @ 2016-04-01 15:34 ` Benjamin Coddington 2016-04-01 15:47 ` [PATCH 0/3] Include OFD lock owners when looking up state Trond Myklebust 2016-04-01 17:17 ` Benjamin Coddington 4 siblings, 0 replies; 16+ messages in thread From: Benjamin Coddington @ 2016-04-01 15:34 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker, Jeff Layton; +Cc: linux-nfs Now that our lock context contains owners for OFD locks we can use those owners to reference an appropriate nfs4_lock_state. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/nfs4state.c | 21 ++++++++++++--------- fs/nfs/pagelist.c | 1 + fs/nfs/write.c | 1 + 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index b7f4509..c602e8f 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -800,14 +800,16 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode) * that is compatible with current->files */ static struct nfs4_lock_state * -__nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner) +__nfs4_find_lock_state(struct nfs4_state *state, + const struct nfs_lockowner *lockowner) { struct nfs4_lock_state *pos; list_for_each_entry(pos, &state->lock_states, ls_locks) { - if (pos->ls_owner != fl_owner) - continue; - atomic_inc(&pos->ls_count); - return pos; + if (pos->ls_owner == lockowner->l_owner_posix || + pos->ls_owner == lockowner->l_owner_ofd) { + atomic_inc(&pos->ls_count); + return pos; + } } return NULL; } @@ -854,10 +856,13 @@ void nfs4_free_lock_state(struct nfs_server *server, struct nfs4_lock_state *lsp static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_owner_t owner) { struct nfs4_lock_state *lsp, *new = NULL; + const struct nfs_lockowner lockowner = { + .l_owner_posix = owner, + }; for(;;) { spin_lock(&state->state_lock); - lsp = __nfs4_find_lock_state(state, owner); + lsp = __nfs4_find_lock_state(state, &lockowner); if (lsp != NULL) break; if (new != NULL) { @@ -942,7 +947,6 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst, const struct nfs_lockowner *lockowner) { struct nfs4_lock_state *lsp; - fl_owner_t fl_owner; int ret = -ENOENT; @@ -952,9 +956,8 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst, if (test_bit(LK_STATE_IN_USE, &state->flags) == 0) goto out; - fl_owner = lockowner->l_owner_posix; spin_lock(&state->state_lock); - lsp = __nfs4_find_lock_state(state, fl_owner); + lsp = __nfs4_find_lock_state(state, lockowner); if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags)) ret = -EIO; else if (lsp != NULL && test_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags) != 0) { diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c index 2b28dff..3579638 100644 --- a/fs/nfs/pagelist.c +++ b/fs/nfs/pagelist.c @@ -859,6 +859,7 @@ static bool nfs_match_lock_context(const struct nfs_lock_context *l1, const struct nfs_lock_context *l2) { return l1->lockowner.l_owner_posix == l2->lockowner.l_owner_posix + && l1->lockowner.l_owner_ofd == l2->lockowner.l_owner_ofd && l1->lockowner.l_pid == l2->lockowner.l_pid; } diff --git a/fs/nfs/write.c b/fs/nfs/write.c index 935244c..2d01d6c 100644 --- a/fs/nfs/write.c +++ b/fs/nfs/write.c @@ -1162,6 +1162,7 @@ int nfs_flush_incompatible(struct file *file, struct page *page) !(list_empty_careful(&flctx->flc_posix) && list_empty_careful(&flctx->flc_flock))) { do_flush |= l_ctx->lockowner.l_owner_posix != current->files + || l_ctx->lockowner.l_owner_ofd != file || l_ctx->lockowner.l_pid != current->tgid; } nfs_release_request(req); -- 1.7.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Include OFD lock owners when looking up state 2016-04-01 15:34 [PATCH 0/3] Include OFD lock owners when looking up state Benjamin Coddington ` (2 preceding siblings ...) 2016-04-01 15:34 ` [PATCH 3/3] NFSv4: use OFD lock owners in lock state lookup Benjamin Coddington @ 2016-04-01 15:47 ` Trond Myklebust 2016-04-01 15:48 ` Benjamin Coddington 2016-04-01 16:09 ` Jeff Layton 2016-04-01 17:17 ` Benjamin Coddington 4 siblings, 2 replies; 16+ messages in thread From: Trond Myklebust @ 2016-04-01 15:47 UTC (permalink / raw) To: Benjamin Coddington; +Cc: Anna Schumaker, Jeff Layton, Linux NFS Mailing List On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > The client sends IO only under the open stateid when using OFD (and flock) > locking instead of the appropriate lock stateid because the nfs_lock_context > only tracks POSIX lockowners, which is the reference to the process' file > table. > > This is a problem for two reasons. The first is that rfc7530, > section-9.1.4.5 states that IO sent by an entity corresponding to the > lock-owner which holds a byte-range lock should be sent under the lock > stateid for that lock. Otherwise, a server enforcing mandatory byte-range > locking might reject that operation. Secondly, not tracking OFD lock owners > means that accounting for IO sent under those owners is broken. That > creates a problem for some work to guarantee an unlock will be sent after > operations scheduled under a lock complete. OK. Can we just kill this in the bud? No support for OFD locks in NFS: this is nuts.... ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Include OFD lock owners when looking up state 2016-04-01 15:47 ` [PATCH 0/3] Include OFD lock owners when looking up state Trond Myklebust @ 2016-04-01 15:48 ` Benjamin Coddington 2016-04-01 16:09 ` Trond Myklebust 2016-04-01 16:09 ` Jeff Layton 1 sibling, 1 reply; 16+ messages in thread From: Benjamin Coddington @ 2016-04-01 15:48 UTC (permalink / raw) To: Trond Myklebust; +Cc: Anna Schumaker, Jeff Layton, Linux NFS Mailing List On Fri, 1 Apr 2016, Trond Myklebust wrote: > On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington > <bcodding@redhat.com> wrote: > > The client sends IO only under the open stateid when using OFD (and flock) > > locking instead of the appropriate lock stateid because the nfs_lock_context > > only tracks POSIX lockowners, which is the reference to the process' file > > table. > > > > This is a problem for two reasons. The first is that rfc7530, > > section-9.1.4.5 states that IO sent by an entity corresponding to the > > lock-owner which holds a byte-range lock should be sent under the lock > > stateid for that lock. Otherwise, a server enforcing mandatory byte-range > > locking might reject that operation. Secondly, not tracking OFD lock owners > > means that accounting for IO sent under those owners is broken. That > > creates a problem for some work to guarantee an unlock will be sent after > > operations scheduled under a lock complete. > > OK. Can we just kill this in the bud? No support for OFD locks in NFS: > this is nuts.... Will you explain why it is nuts? That would be helpful for me. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Include OFD lock owners when looking up state 2016-04-01 15:48 ` Benjamin Coddington @ 2016-04-01 16:09 ` Trond Myklebust 2016-04-01 16:19 ` Jeff Layton 2016-04-01 16:35 ` Benjamin Coddington 0 siblings, 2 replies; 16+ messages in thread From: Trond Myklebust @ 2016-04-01 16:09 UTC (permalink / raw) To: Benjamin Coddington; +Cc: Anna Schumaker, Jeff Layton, Linux NFS Mailing List On Fri, Apr 1, 2016 at 11:48 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > On Fri, 1 Apr 2016, Trond Myklebust wrote: > >> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington >> <bcodding@redhat.com> wrote: >> > The client sends IO only under the open stateid when using OFD (and flock) >> > locking instead of the appropriate lock stateid because the nfs_lock_context >> > only tracks POSIX lockowners, which is the reference to the process' file >> > table. >> > >> > This is a problem for two reasons. The first is that rfc7530, >> > section-9.1.4.5 states that IO sent by an entity corresponding to the >> > lock-owner which holds a byte-range lock should be sent under the lock >> > stateid for that lock. Otherwise, a server enforcing mandatory byte-range >> > locking might reject that operation. Secondly, not tracking OFD lock owners >> > means that accounting for IO sent under those owners is broken. That >> > creates a problem for some work to guarantee an unlock will be sent after >> > operations scheduled under a lock complete. >> >> OK. Can we just kill this in the bud? No support for OFD locks in NFS: >> this is nuts.... > > Will you explain why it is nuts? That would be helpful for me. The point of the OFD crap was that they should work exactly like POSIX locks except for the unlock-on-close, the latter being managed by the VFS layer. If we have to make loads of changes to NFS in order to change the tracking of lock owners, then the design itself is broken, and needs to be fixed. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Include OFD lock owners when looking up state 2016-04-01 16:09 ` Trond Myklebust @ 2016-04-01 16:19 ` Jeff Layton 2016-04-01 20:24 ` Frank Filz 2016-04-01 16:35 ` Benjamin Coddington 1 sibling, 1 reply; 16+ messages in thread From: Jeff Layton @ 2016-04-01 16:19 UTC (permalink / raw) To: Trond Myklebust Cc: Benjamin Coddington, Anna Schumaker, Linux NFS Mailing List On Fri, 1 Apr 2016 12:09:31 -0400 Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Fri, Apr 1, 2016 at 11:48 AM, Benjamin Coddington > <bcodding@redhat.com> wrote: > > On Fri, 1 Apr 2016, Trond Myklebust wrote: > > > >> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington > >> <bcodding@redhat.com> wrote: > >> > The client sends IO only under the open stateid when using OFD (and flock) > >> > locking instead of the appropriate lock stateid because the nfs_lock_context > >> > only tracks POSIX lockowners, which is the reference to the process' file > >> > table. > >> > > >> > This is a problem for two reasons. The first is that rfc7530, > >> > section-9.1.4.5 states that IO sent by an entity corresponding to the > >> > lock-owner which holds a byte-range lock should be sent under the lock > >> > stateid for that lock. Otherwise, a server enforcing mandatory byte-range > >> > locking might reject that operation. Secondly, not tracking OFD lock owners > >> > means that accounting for IO sent under those owners is broken. That > >> > creates a problem for some work to guarantee an unlock will be sent after > >> > operations scheduled under a lock complete. > >> > >> OK. Can we just kill this in the bud? No support for OFD locks in NFS: > >> this is nuts.... > > > > Will you explain why it is nuts? That would be helpful for me. > > The point of the OFD crap was that they should work exactly like POSIX > locks except for the unlock-on-close, the latter being managed by the > VFS layer. If we have to make loads of changes to NFS in order to > change the tracking of lock owners, then the design itself is broken, > and needs to be fixed. No, there were other reasons for them as well. For instance, traditional POSIX locks are useless for serializing betwee threads within the same process because the lock owner is always the same regardless of which thread you're using. With OFD locks, this is possible if each thread has its own file description. -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 0/3] Include OFD lock owners when looking up state 2016-04-01 16:19 ` Jeff Layton @ 2016-04-01 20:24 ` Frank Filz 0 siblings, 0 replies; 16+ messages in thread From: Frank Filz @ 2016-04-01 20:24 UTC (permalink / raw) To: 'Jeff Layton', 'Trond Myklebust' Cc: 'Benjamin Coddington', 'Anna Schumaker', 'Linux NFS Mailing List' > > On Fri, Apr 1, 2016 at 11:48 AM, Benjamin Coddington > > <bcodding@redhat.com> wrote: > > > On Fri, 1 Apr 2016, Trond Myklebust wrote: > > > > > >> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington > > >> <bcodding@redhat.com> wrote: > > >> > The client sends IO only under the open stateid when using OFD > > >> > (and flock) locking instead of the appropriate lock stateid > > >> > because the nfs_lock_context only tracks POSIX lockowners, which > > >> > is the reference to the process' file table. > > >> > > > >> > This is a problem for two reasons. The first is that rfc7530, > > >> > section-9.1.4.5 states that IO sent by an entity corresponding to > > >> > the lock-owner which holds a byte-range lock should be sent under > > >> > the lock stateid for that lock. Otherwise, a server enforcing > > >> > mandatory byte-range locking might reject that operation. > > >> > Secondly, not tracking OFD lock owners means that accounting for > > >> > IO sent under those owners is broken. That creates a problem for > > >> > some work to guarantee an unlock will be sent after operations > scheduled under a lock complete. > > >> > > >> OK. Can we just kill this in the bud? No support for OFD locks in NFS: > > >> this is nuts.... > > > > > > Will you explain why it is nuts? That would be helpful for me. > > > > The point of the OFD crap was that they should work exactly like POSIX > > locks except for the unlock-on-close, the latter being managed by the > > VFS layer. If we have to make loads of changes to NFS in order to > > change the tracking of lock owners, then the design itself is broken, > > and needs to be fixed. > > > No, there were other reasons for them as well. > > For instance, traditional POSIX locks are useless for serializing betwee > threads within the same process because the lock owner is always the same > regardless of which thread you're using. With OFD locks, this is possible if > each thread has its own file description. Which is a feature humongously useful for Ganesha... Not that Ganesha will ever use OFD locks over NFS... I have used OFD locks over NFS to make sure that Ganesha handles one open-owner to many lock-owners correctly. Of course it may be a real question if anyone other than Ganesha (and maybe Samba) will ever use OFD locks since they aren't a POSIX standard... Frank --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Include OFD lock owners when looking up state 2016-04-01 16:09 ` Trond Myklebust 2016-04-01 16:19 ` Jeff Layton @ 2016-04-01 16:35 ` Benjamin Coddington 1 sibling, 0 replies; 16+ messages in thread From: Benjamin Coddington @ 2016-04-01 16:35 UTC (permalink / raw) To: Trond Myklebust; +Cc: Anna Schumaker, Jeff Layton, Linux NFS Mailing List On Fri, 1 Apr 2016, Trond Myklebust wrote: > On Fri, Apr 1, 2016 at 11:48 AM, Benjamin Coddington > <bcodding@redhat.com> wrote: > > On Fri, 1 Apr 2016, Trond Myklebust wrote: > > > >> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington > >> <bcodding@redhat.com> wrote: > >> > The client sends IO only under the open stateid when using OFD (and flock) > >> > locking instead of the appropriate lock stateid because the nfs_lock_context > >> > only tracks POSIX lockowners, which is the reference to the process' file > >> > table. > >> > > >> > This is a problem for two reasons. The first is that rfc7530, > >> > section-9.1.4.5 states that IO sent by an entity corresponding to the > >> > lock-owner which holds a byte-range lock should be sent under the lock > >> > stateid for that lock. Otherwise, a server enforcing mandatory byte-range > >> > locking might reject that operation. Secondly, not tracking OFD lock owners > >> > means that accounting for IO sent under those owners is broken. That > >> > creates a problem for some work to guarantee an unlock will be sent after > >> > operations scheduled under a lock complete. > >> > >> OK. Can we just kill this in the bud? No support for OFD locks in NFS: > >> this is nuts.... > > > > Will you explain why it is nuts? That would be helpful for me. > > The point of the OFD crap was that they should work exactly like POSIX > locks except for the unlock-on-close, the latter being managed by the > VFS layer. If we have to make loads of changes to NFS in order to > change the tracking of lock owners, then the design itself is broken, > and needs to be fixed. It seems your objection is more about trying to combine the two different models of a lock's ownership or boundaries from the VFS side, where in NFS there's been traditionally only one model used. The reason that OFD locks require changes to work on NFS is because the owners (the context of a lock) are different, and NFS right now assumes that the boundaries of lock ownership are always going to be a shared file table. Assuming that the boundaries of a lock will only ever be entities with shared file tables might be difficult to support long term. What happens when applications start using OFD locks (or other future locks that might allow user-defined locking contexts) and then later we discover applications won't work properly on NFS? How can we fix this once for NFS? NFS could be fixed if it didn't have to keep track of the lock context, if the lock context were opaque. Should there be a way to look up a reference to a locking context without having to keep track of files or file tables or anything else? Ben ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Include OFD lock owners when looking up state 2016-04-01 15:47 ` [PATCH 0/3] Include OFD lock owners when looking up state Trond Myklebust 2016-04-01 15:48 ` Benjamin Coddington @ 2016-04-01 16:09 ` Jeff Layton 1 sibling, 0 replies; 16+ messages in thread From: Jeff Layton @ 2016-04-01 16:09 UTC (permalink / raw) To: Trond Myklebust Cc: Benjamin Coddington, Anna Schumaker, Linux NFS Mailing List On Fri, 1 Apr 2016 11:47:10 -0400 Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington > <bcodding@redhat.com> wrote: > > The client sends IO only under the open stateid when using OFD (and flock) > > locking instead of the appropriate lock stateid because the nfs_lock_context > > only tracks POSIX lockowners, which is the reference to the process' file > > table. > > > > This is a problem for two reasons. The first is that rfc7530, > > section-9.1.4.5 states that IO sent by an entity corresponding to the > > lock-owner which holds a byte-range lock should be sent under the lock > > stateid for that lock. Otherwise, a server enforcing mandatory byte-range > > locking might reject that operation. Secondly, not tracking OFD lock owners > > means that accounting for IO sent under those owners is broken. That > > creates a problem for some work to guarantee an unlock will be sent after > > operations scheduled under a lock complete. > > OK. Can we just kill this in the bud? No support for OFD locks in NFS: > this is nuts.... Why wouldn't there be? OFD locks seem quite sane (more so than traditional POSIX locks anyway), and the opaque v4 lockowner model should work with OFD locks just fine... -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Include OFD lock owners when looking up state 2016-04-01 15:34 [PATCH 0/3] Include OFD lock owners when looking up state Benjamin Coddington ` (3 preceding siblings ...) 2016-04-01 15:47 ` [PATCH 0/3] Include OFD lock owners when looking up state Trond Myklebust @ 2016-04-01 17:17 ` Benjamin Coddington 4 siblings, 0 replies; 16+ messages in thread From: Benjamin Coddington @ 2016-04-01 17:17 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker, Jeff Layton; +Cc: linux-nfs kbuild robot noticed I made a mistake doing a quick rebase. I'll send a V2 for completeness even if it is disliked. I hope we can keep discussing it.. Ben On Fri, 1 Apr 2016, Benjamin Coddington wrote: > The client sends IO only under the open stateid when using OFD (and flock) > locking instead of the appropriate lock stateid because the nfs_lock_context > only tracks POSIX lockowners, which is the reference to the process' file > table. > > This is a problem for two reasons. The first is that rfc7530, > section-9.1.4.5 states that IO sent by an entity corresponding to the > lock-owner which holds a byte-range lock should be sent under the lock > stateid for that lock. Otherwise, a server enforcing mandatory byte-range > locking might reject that operation. Secondly, not tracking OFD lock owners > means that accounting for IO sent under those owners is broken. That > creates a problem for some work to guarantee an unlock will be sent after > operations scheduled under a lock complete. > > To fix this, this set starts tracking the OFD lock owner within the > nfs_lock_context. Unique nfs_lock_contexts are created on distinct > combinations of current->files, struct file, and pid. In order to do this, > some re-arrangement of when to create or lookup the nfs_lock_context was > needed since the struct file pointer must be included during that creation > or lookup. > > It is possible for a process to hold both an OFD and a POSIX lock on a file > as long as they do not conflict. There would be two stateids that could be > selected for IO operations on that file. In that case only the first > stateid found to match the current lock context will ever be used. The > result of this is that mixed OFD and POSIX locks within the same process on > a server enforcing mandatory locking should be avoided. The fix for this > probably would require a byte-range lookup of stateid when scheduling an IO > operation, and the ability to split IO that crossed lock ranges with > different owners. > > Comments and critique is welcomed. > > Benjamin Coddington (3): > NFS: add get_nfs_lock_context, find_nfs_lock_context > NFS: add OFD lock owners to nfs_lock_context > NFSv4: use OFD lock owners in lock state lookup > > fs/nfs/direct.c | 8 ++++---- > fs/nfs/file.c | 2 +- > fs/nfs/fscache-index.c | 4 ++-- > fs/nfs/fscache.h | 12 ++++++------ > fs/nfs/inode.c | 32 ++++++++++++++++++++++---------- > fs/nfs/nfs42proc.c | 8 ++++---- > fs/nfs/nfs4proc.c | 3 ++- > fs/nfs/nfs4state.c | 21 ++++++++++++--------- > fs/nfs/pagelist.c | 22 ++++++++-------------- > fs/nfs/read.c | 38 +++++++++++++++++++++++--------------- > fs/nfs/write.c | 17 ++++++++++------- > include/linux/nfs_fs.h | 8 +++++--- > include/linux/nfs_page.h | 2 +- > 13 files changed, 100 insertions(+), 77 deletions(-) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-04-01 20:24 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-01 15:34 [PATCH 0/3] Include OFD lock owners when looking up state Benjamin Coddington 2016-04-01 15:34 ` [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context Benjamin Coddington 2016-04-01 16:45 ` kbuild test robot 2016-04-01 17:03 ` kbuild test robot 2016-04-01 15:34 ` [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context Benjamin Coddington 2016-04-01 16:17 ` kbuild test robot 2016-04-01 16:55 ` kbuild test robot 2016-04-01 15:34 ` [PATCH 3/3] NFSv4: use OFD lock owners in lock state lookup Benjamin Coddington 2016-04-01 15:47 ` [PATCH 0/3] Include OFD lock owners when looking up state Trond Myklebust 2016-04-01 15:48 ` Benjamin Coddington 2016-04-01 16:09 ` Trond Myklebust 2016-04-01 16:19 ` Jeff Layton 2016-04-01 20:24 ` Frank Filz 2016-04-01 16:35 ` Benjamin Coddington 2016-04-01 16:09 ` Jeff Layton 2016-04-01 17:17 ` Benjamin Coddington
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.