LinuxLists.cc - [PATCH 0/3] Include OFD lock owners when looking up state

2016-04-01 15:34:52

Subject: [PATCH 0/3] Include OFD lock owners when looking up state

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(-)

2016-04-01 15:34:52

by Benjamin Coddington

[permalink] [raw]

Subject: [PATCH 3/3] NFSv4: use OFD lock owners in lock state lookup

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 <[email protected]>
---
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

2016-04-01 15:34:52

by Benjamin Coddington

[permalink] [raw]

Subject: [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context

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 <[email protected]>
---
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

2016-04-01 15:34:52

by Benjamin Coddington

[permalink] [raw]

Subject: [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context

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 <[email protected]>
---
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

2016-04-01 15:47:10

by Trond Myklebust

[permalink] [raw]

Subject: Re: [PATCH 0/3] Include OFD lock owners when looking up state

On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington
<[email protected]> 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....

2016-04-01 15:48:45

by Benjamin Coddington

[permalink] [raw]

Subject: Re: [PATCH 0/3] Include OFD lock owners when looking up state

On Fri, 1 Apr 2016, Trond Myklebust wrote:

> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington
> <[email protected]> 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.

2016-04-01 16:09:13

by Jeff Layton

[permalink] [raw]

Subject: Re: [PATCH 0/3] Include OFD lock owners when looking up state

On Fri, 1 Apr 2016 11:47:10 -0400
Trond Myklebust <[email protected]> wrote:

> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington
> <[email protected]> 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 <[email protected]>

2016-04-01 16:09:31

by Trond Myklebust

[permalink] [raw]

Subject: Re: [PATCH 0/3] Include OFD lock owners when looking up state

On Fri, Apr 1, 2016 at 11:48 AM, Benjamin Coddington
<[email protected]> wrote:
> On Fri, 1 Apr 2016, Trond Myklebust wrote:
>
>> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington
>> <[email protected]> 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.

2016-04-01 16:17:04

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context

Attachments:

(No filename) (3.40 kB)
.config.gz (44.72 kB)
Download all attachments

2016-04-01 16:19:24

by Jeff Layton

[permalink] [raw]

Subject: Re: [PATCH 0/3] Include OFD lock owners when looking up state

On Fri, 1 Apr 2016 12:09:31 -0400
Trond Myklebust <[email protected]> wrote:

> On Fri, Apr 1, 2016 at 11:48 AM, Benjamin Coddington
> <[email protected]> wrote:
> > On Fri, 1 Apr 2016, Trond Myklebust wrote:
> >
> >> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington
> >> <[email protected]> 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 <[email protected]>

2016-04-01 16:35:25

by Benjamin Coddington

[permalink] [raw]

Subject: Re: [PATCH 0/3] Include OFD lock owners when looking up state

On Fri, 1 Apr 2016, Trond Myklebust wrote:

> On Fri, Apr 1, 2016 at 11:48 AM, Benjamin Coddington
> <[email protected]> wrote:
> > On Fri, 1 Apr 2016, Trond Myklebust wrote:
> >
> >> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington
> >> <[email protected]> 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

2016-04-01 16:45:39

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context

Attachments:

(No filename) (3.71 kB)
.config.gz (17.82 kB)
Download all attachments

2016-04-01 16:55:40

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 2/3] NFS: add OFD lock owners to nfs_lock_context

Attachments:

(No filename) (4.06 kB)
.config.gz (28.67 kB)
Download all attachments

2016-04-01 17:05:32

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/3] NFS: add get_nfs_lock_context, find_nfs_lock_context

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

Attachments:

(No filename) (7.60 kB)
.config.gz (44.86 kB)
Download all attachments

2016-04-01 17:17:06

by Benjamin Coddington

[permalink] [raw]

Subject: Re: [PATCH 0/3] Include OFD lock owners when looking up state

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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2016-04-01 20:24:57

by Frank Filz

[permalink] [raw]

Subject: RE: [PATCH 0/3] Include OFD lock owners when looking up state

> > On Fri, Apr 1, 2016 at 11:48 AM, Benjamin Coddington
> > <[email protected]> wrote:
> > > On Fri, 1 Apr 2016, Trond Myklebust wrote:
> > >
> > >> On Fri, Apr 1, 2016 at 11:34 AM, Benjamin Coddington
> > >> <[email protected]> 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