[PATCH 0/5] NFSv4: Fix stateid used when flock locks in use.

[PATCH 0/5] NFSv4: Fix stateid used when flock locks in use.

[NeilBrown]

I received no comments for my RFC series I sent a week ago, so I'll
assume no-one objects :-)

I've revised the series a little.  In particular I just add a
lockowner rather than a whole nfs_lock_context to the open_context.
I've also added to patches which combine to remove the nfs_lockowner
structure.

I think this is ready to be queued for -next

Thanks,
NeilBrown


---

NeilBrown (5):
      NFS: remove l_pid field from nfs_lockowner
      NFSv4: add flock_owner to open context
      NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner
      NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one
      NFS: discard nfs_lockowner structure.


 fs/nfs/dir.c           |    6 +++---
 fs/nfs/inode.c         |   14 +++++++-------
 fs/nfs/nfs4_fs.h       |    2 +-
 fs/nfs/nfs4file.c      |    2 +-
 fs/nfs/nfs4proc.c      |   14 +++-----------
 fs/nfs/nfs4state.c     |   34 +++++++++++++++++++++-------------
 fs/nfs/pagelist.c      |    3 +--
 fs/nfs/write.c         |    3 +--
 include/linux/nfs_fs.h |   10 +++-------
 9 files changed, 41 insertions(+), 47 deletions(-)

--
Signature

[PATCH 1/5] NFS: remove l_pid field from nfs_lockowner

[NeilBrown]

this field is not used in any important way and probably should
have been removed by

Commit: 8003d3c4aaa5 ("nfs4: treat lock owners as opaque values")

which removed the pid argument from nfs4_get_lock_state.

Except in unusual and uninteresting cases, two threads with the same
->tgid will have the same ->files pointer, so keeping them both
for comparison brings no benefit.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/inode.c         |    3 ---
 fs/nfs/nfs4proc.c      |    1 -
 fs/nfs/pagelist.c      |    3 +--
 fs/nfs/write.c         |    3 +--
 include/linux/nfs_fs.h |    1 -
 5 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index bf4ec5ecc97e..1752fc7c0024 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -703,7 +703,6 @@ static void nfs_init_lock_context(struct nfs_lock_context *l_ctx)
 {
 	atomic_set(&l_ctx->count, 1);
 	l_ctx->lockowner.l_owner = current->files;
-	l_ctx->lockowner.l_pid = current->tgid;
 	INIT_LIST_HEAD(&l_ctx->list);
 	atomic_set(&l_ctx->io_count, 0);
 }
@@ -716,8 +715,6 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context
 	do {
 		if (pos->lockowner.l_owner != current->files)
 			continue;
-		if (pos->lockowner.l_pid != current->tgid)
-			continue;
 		atomic_inc(&pos->count);
 		return pos;
 	} while ((pos = list_entry(pos->list.next, typeof(*pos), list)) != head);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 0e327528a3ce..b7df3ef84fc3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2766,7 +2766,6 @@ static int _nfs4_do_setattr(struct inode *inode,
 	} else if (truncate && state != NULL) {
 		struct nfs_lockowner lockowner = {
 			.l_owner = current->files,
-			.l_pid = current->tgid,
 		};
 		if (!nfs4_valid_open_stateid(state))
 			return -EBADF;
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 965db474f4b0..161f8b13bbaa 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -867,8 +867,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
-		&& l1->lockowner.l_pid == l2->lockowner.l_pid;
+	return l1->lockowner.l_owner == l2->lockowner.l_owner;
 }
 
 /**
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 53211838f72a..4d5897e6d6cb 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1151,8 +1151,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
-				|| l_ctx->lockowner.l_pid != current->tgid;
+			do_flush |= l_ctx->lockowner.l_owner != current->files;
 		}
 		nfs_release_request(req);
 		if (!do_flush)
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 810124b33327..bf8a713c45b4 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -57,7 +57,6 @@ struct nfs_access_entry {
 
 struct nfs_lockowner {
 	fl_owner_t l_owner;
-	pid_t l_pid;
 };
 
 struct nfs_lock_context {

[PATCH 2/5] NFSv4: add flock_owner to open context

[NeilBrown]

An open file description (struct file) in a given process can be
associated with two different lock owners.

It can have a Posix lock owner which will be different in each process
that has a fd on the file.
It can have a Flock owner which will be the same in all processes.

When searching for a lock stateid to use, we need to consider both of these
owners

So add a new "flock_owner" to the "nfs_open_context" (of which there
is one for each open file description).

This flock_owner does not need to be reference-counted as there is a
1-1 relation between 'struct file' and nfs open contexts,
and it will never be part of a list of contexts.  So there is no need
for a 'flock_context' - just the owner is enough.

The io_count included in the (Posix) lock_context provides no
guarantee that all read-aheads that could use the state have
completed, so not supporting it for flock locks in not a serious
problem.  Synchronization between flock and read-ahead can be added
later if needed.

When creating an open_context for a non-openning create call, we don't have
a 'struct file' to pass in, so the lock context gets initialized with
a NULL owner, but this will never be used.

The flock_owner is not used at all in this patch, that will come later.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/dir.c           |    6 +++---
 fs/nfs/inode.c         |    7 +++++--
 fs/nfs/nfs4file.c      |    2 +-
 fs/nfs/nfs4proc.c      |    2 +-
 include/linux/nfs_fs.h |    3 ++-
 5 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 06e0bf092ba9..d5b87b28010d 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1453,9 +1453,9 @@ static fmode_t flags_to_mode(int flags)
 	return res;
 }
 
-static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags)
+static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags, struct file *filp)
 {
-	return alloc_nfs_open_context(dentry, flags_to_mode(open_flags));
+	return alloc_nfs_open_context(dentry, flags_to_mode(open_flags), filp);
 }
 
 static int do_open(struct inode *inode, struct file *filp)
@@ -1540,7 +1540,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 			return finish_no_open(file, dentry);
 	}
 
-	ctx = create_nfs_open_context(dentry, open_flags);
+	ctx = create_nfs_open_context(dentry, open_flags, file);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 1752fc7c0024..863c25d99a9b 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -796,7 +796,9 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync)
 }
 EXPORT_SYMBOL_GPL(nfs_close_context);
 
-struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode)
+struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry,
+						fmode_t f_mode,
+						struct file *filp)
 {
 	struct nfs_open_context *ctx;
 	struct rpc_cred *cred = rpc_lookup_cred();
@@ -815,6 +817,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;
+	ctx->flock_owner = filp;
 	nfs_init_lock_context(&ctx->lock_context);
 	ctx->lock_context.open_context = ctx;
 	INIT_LIST_HEAD(&ctx->list);
@@ -939,7 +942,7 @@ int nfs_open(struct inode *inode, struct file *filp)
 {
 	struct nfs_open_context *ctx;
 
-	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode, filp);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 	nfs_file_set_open_context(filp, ctx);
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 89a77950e0b0..0efba77789b9 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, struct file *filp)
 	parent = dget_parent(dentry);
 	dir = d_inode(parent);
 
-	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode, filp);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index b7df3ef84fc3..422ed5ac4efe 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3792,7 +3792,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 	struct nfs4_state *state;
 	int status = 0;
 
-	ctx = alloc_nfs_open_context(dentry, FMODE_READ);
+	ctx = alloc_nfs_open_context(dentry, FMODE_READ, NULL);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index bf8a713c45b4..c90bf92d2b09 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -70,6 +70,7 @@ struct nfs_lock_context {
 struct nfs4_state;
 struct nfs_open_context {
 	struct nfs_lock_context lock_context;
+	fl_owner_t *flock_owner;
 	struct dentry *dentry;
 	struct rpc_cred *cred;
 	struct nfs4_state *state;
@@ -357,7 +358,7 @@ extern void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr,
 extern struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx);
 extern void put_nfs_open_context(struct nfs_open_context *ctx);
 extern struct nfs_open_context *nfs_find_open_context(struct inode *inode, struct rpc_cred *cred, fmode_t mode);
-extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode);
+extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode, struct file *filp);
 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);

[PATCH 3/5] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner

[NeilBrown]

The only time that a lock_context is not available is in setattr.
In this case, we want to find a lock context relevant to the process if there is one.
The fallback can easily be handled at a lower level.

So change nfs4_select_rw_stateid to take a lock_context, passing NULL from _nfs4_do_setattr.
nfs4_copy_lock_state() also now takes a lock_context, and falls back to searching
for "owner == current->files" if not lock_context is given.

Note that nfs4_set_rw_stateid is *always* passed a non-NULL l_ctx, so the
fact that we remove the NULL test there does not change correctness.

This change is preparation for correctly support flock stateids.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/nfs4_fs.h   |    2 +-
 fs/nfs/nfs4proc.c  |   11 ++---------
 fs/nfs/nfs4state.c |   19 +++++++++++--------
 3 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 9bf64eacba5b..3f0e459f2499 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -445,7 +445,7 @@ extern void nfs41_handle_server_scope(struct nfs_client *,
 extern void nfs4_put_lock_state(struct nfs4_lock_state *lsp);
 extern int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl);
 extern int nfs4_select_rw_stateid(struct nfs4_state *, fmode_t,
-		const struct nfs_lockowner *, nfs4_stateid *,
+		const struct nfs_lock_context *, nfs4_stateid *,
 		struct rpc_cred **);
 
 extern struct nfs_seqid *nfs_alloc_seqid(struct nfs_seqid_counter *counter, gfp_t gfp_mask);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 422ed5ac4efe..6820c44aebe4 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2764,12 +2764,9 @@ static int _nfs4_do_setattr(struct inode *inode,
 	if (nfs4_copy_delegation_stateid(inode, fmode, &arg->stateid, &delegation_cred)) {
 		/* Use that stateid */
 	} else if (truncate && state != NULL) {
-		struct nfs_lockowner lockowner = {
-			.l_owner = current->files,
-		};
 		if (!nfs4_valid_open_stateid(state))
 			return -EBADF;
-		if (nfs4_select_rw_stateid(state, FMODE_WRITE, &lockowner,
+		if (nfs4_select_rw_stateid(state, FMODE_WRITE, NULL,
 				&arg->stateid, &delegation_cred) == -EIO)
 			return -EBADF;
 	} else
@@ -4362,11 +4359,7 @@ int nfs4_set_rw_stateid(nfs4_stateid *stateid,
 		const struct nfs_lock_context *l_ctx,
 		fmode_t fmode)
 {
-	const struct nfs_lockowner *lockowner = NULL;
-
-	if (l_ctx != NULL)
-		lockowner = &l_ctx->lockowner;
-	return nfs4_select_rw_stateid(ctx->state, fmode, lockowner, stateid, NULL);
+	return nfs4_select_rw_stateid(ctx->state, fmode, l_ctx, stateid, NULL);
 }
 EXPORT_SYMBOL_GPL(nfs4_set_rw_stateid);
 
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index cada00aa5096..94a6631e7938 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -939,20 +939,23 @@ int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl)
 
 static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
 		struct nfs4_state *state,
-		const struct nfs_lockowner *lockowner)
+		const struct nfs_lock_context *l_ctx)
 {
+	/*
+	 * If l_ctx is NULL, then this request comes from setattr
+	 * and we can choose a lock context relevant for the current process
+	 */
 	struct nfs4_lock_state *lsp;
 	fl_owner_t fl_owner;
 	int ret = -ENOENT;
 
-
-	if (lockowner == NULL)
-		goto out;
-
 	if (test_bit(LK_STATE_IN_USE, &state->flags) == 0)
 		goto out;
 
-	fl_owner = lockowner->l_owner;
+	if (l_ctx == NULL)
+		fl_owner = current->files;
+	else
+		fl_owner = l_ctx->lockowner.l_owner;
 	spin_lock(&state->state_lock);
 	lsp = __nfs4_find_lock_state(state, fl_owner);
 	if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags))
@@ -986,14 +989,14 @@ static void nfs4_copy_open_stateid(nfs4_stateid *dst, struct nfs4_state *state)
  * requests.
  */
 int nfs4_select_rw_stateid(struct nfs4_state *state,
-		fmode_t fmode, const struct nfs_lockowner *lockowner,
+		fmode_t fmode, const struct nfs_lock_context *l_ctx,
 		nfs4_stateid *dst, struct rpc_cred **cred)
 {
 	int ret;
 
 	if (cred != NULL)
 		*cred = NULL;
-	ret = nfs4_copy_lock_stateid(dst, state, lockowner);
+	ret = nfs4_copy_lock_stateid(dst, state, l_ctx);
 	if (ret == -EIO)
 		/* A lost lock - don't even consider delegations */
 		goto out;

[PATCH 4/5] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one

[NeilBrown]

A process can have two possible lock owner for a given open file:
a per-process Posix lock owner and a per-open-file flock owner
Use both of these when searching for a suitable stateid to use.

With this patch, READ/WRITE requests will use the correct stateid
if a flock lock is active.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/nfs4state.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 94a6631e7938..fbef37fd5704 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -800,11 +800,13 @@ 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,
+		       fl_owner_t fl_owner, fl_owner_t fl_owner2)
 {
 	struct nfs4_lock_state *pos;
 	list_for_each_entry(pos, &state->lock_states, ls_locks) {
-		if (pos->ls_owner != fl_owner)
+		if (pos->ls_owner != fl_owner &&
+		    pos->ls_owner != fl_owner2)
 			continue;
 		atomic_inc(&pos->ls_count);
 		return pos;
@@ -857,7 +859,7 @@ static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_
 	
 	for(;;) {
 		spin_lock(&state->state_lock);
-		lsp = __nfs4_find_lock_state(state, owner);
+		lsp = __nfs4_find_lock_state(state, owner, 0);
 		if (lsp != NULL)
 			break;
 		if (new != NULL) {
@@ -946,18 +948,21 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
 	 * and we can choose a lock context relevant for the current process
 	 */
 	struct nfs4_lock_state *lsp;
-	fl_owner_t fl_owner;
+	fl_owner_t fl_owner, fl_flock_owner;
 	int ret = -ENOENT;
 
 	if (test_bit(LK_STATE_IN_USE, &state->flags) == 0)
 		goto out;
 
-	if (l_ctx == NULL)
+	if (l_ctx == NULL) {
 		fl_owner = current->files;
-	else
+		fl_flock_owner = 0;
+	} else {
 		fl_owner = l_ctx->lockowner.l_owner;
+		fl_flock_owner = l_ctx->open_context->flock_owner;
+	}
 	spin_lock(&state->state_lock);
-	lsp = __nfs4_find_lock_state(state, fl_owner);
+	lsp = __nfs4_find_lock_state(state, fl_owner, fl_flock_owner);
 	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) {

[PATCH 5/5] NFS: discard nfs_lockowner structure.

[NeilBrown]

It now has only one field and is only used in one structure.
So replaced it in that structure by the field it contains.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/inode.c         |    4 ++--
 fs/nfs/nfs4state.c     |    2 +-
 fs/nfs/pagelist.c      |    2 +-
 fs/nfs/write.c         |    2 +-
 include/linux/nfs_fs.h |    6 +-----
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 863c25d99a9b..dd64113908dd 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -702,7 +702,7 @@ EXPORT_SYMBOL_GPL(nfs_getattr);
 static void nfs_init_lock_context(struct nfs_lock_context *l_ctx)
 {
 	atomic_set(&l_ctx->count, 1);
-	l_ctx->lockowner.l_owner = current->files;
+	l_ctx->lockowner = current->files;
 	INIT_LIST_HEAD(&l_ctx->list);
 	atomic_set(&l_ctx->io_count, 0);
 }
@@ -713,7 +713,7 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context
 	struct nfs_lock_context *pos = head;
 
 	do {
-		if (pos->lockowner.l_owner != current->files)
+		if (pos->lockowner != current->files)
 			continue;
 		atomic_inc(&pos->count);
 		return pos;
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index fbef37fd5704..4763db88c1d4 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -958,7 +958,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
 		fl_owner = current->files;
 		fl_flock_owner = 0;
 	} else {
-		fl_owner = l_ctx->lockowner.l_owner;
+		fl_owner = l_ctx->lockowner;
 		fl_flock_owner = l_ctx->open_context->flock_owner;
 	}
 	spin_lock(&state->state_lock);
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 161f8b13bbaa..6e629b856a00 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -867,7 +867,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 == l2->lockowner;
 }
 
 /**
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 4d5897e6d6cb..6e761f3f4cbf 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1151,7 +1151,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 != current->files;
 		}
 		nfs_release_request(req);
 		if (!do_flush)
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index c90bf92d2b09..d3efb97db6db 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -55,15 +55,11 @@ struct nfs_access_entry {
 	struct rcu_head		rcu_head;
 };
 
-struct nfs_lockowner {
-	fl_owner_t l_owner;
-};
-
 struct nfs_lock_context {
 	atomic_t count;
 	struct list_head list;
 	struct nfs_open_context *open_context;
-	struct nfs_lockowner lockowner;
+	fl_owner_t lockowner;
 	atomic_t io_count;
 };
 

[PATCH 2/5 - version 2] NFSv4: add flock_owner to open context

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5927 bytes --]


An open file description (struct file) in a given process can be
associated with two different lock owners.

It can have a Posix lock owner which will be different in each process
that has a fd on the file.
It can have a Flock owner which will be the same in all processes.

When searching for a lock stateid to use, we need to consider both of these
owners

So add a new "flock_owner" to the "nfs_open_context" (of which there
is one for each open file description).

This flock_owner does not need to be reference-counted as there is a
1-1 relation between 'struct file' and nfs open contexts,
and it will never be part of a list of contexts.  So there is no need
for a 'flock_context' - just the owner is enough.

The io_count included in the (Posix) lock_context provides no
guarantee that all read-aheads that could use the state have
completed, so not supporting it for flock locks in not a serious
problem.  Synchronization between flock and read-ahead can be added
later if needed.

When creating an open_context for a non-openning create call, we don't have
a 'struct file' to pass in, so the lock context gets initialized with
a NULL owner, but this will never be used.

The flock_owner is not used at all in this patch, that will come later.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/dir.c           | 6 +++---
 fs/nfs/inode.c         | 7 +++++--
 fs/nfs/nfs4file.c      | 2 +-
 fs/nfs/nfs4proc.c      | 2 +-
 include/linux/nfs_fs.h | 3 ++-
 5 files changed, 12 insertions(+), 8 deletions(-)

Sorry, two little errors in previous version....

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 06e0bf092ba9..d5b87b28010d 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1453,9 +1453,9 @@ static fmode_t flags_to_mode(int flags)
 	return res;
 }
 
-static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags)
+static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags, struct file *filp)
 {
-	return alloc_nfs_open_context(dentry, flags_to_mode(open_flags));
+	return alloc_nfs_open_context(dentry, flags_to_mode(open_flags), filp);
 }
 
 static int do_open(struct inode *inode, struct file *filp)
@@ -1540,7 +1540,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 			return finish_no_open(file, dentry);
 	}
 
-	ctx = create_nfs_open_context(dentry, open_flags);
+	ctx = create_nfs_open_context(dentry, open_flags, file);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 1752fc7c0024..2138027eaba9 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -796,7 +796,9 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync)
 }
 EXPORT_SYMBOL_GPL(nfs_close_context);
 
-struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode)
+struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry,
+						fmode_t f_mode,
+						struct file *filp)
 {
 	struct nfs_open_context *ctx;
 	struct rpc_cred *cred = rpc_lookup_cred();
@@ -815,6 +817,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;
+	ctx->flock_owner = (fl_owner_t)filp;
 	nfs_init_lock_context(&ctx->lock_context);
 	ctx->lock_context.open_context = ctx;
 	INIT_LIST_HEAD(&ctx->list);
@@ -939,7 +942,7 @@ int nfs_open(struct inode *inode, struct file *filp)
 {
 	struct nfs_open_context *ctx;
 
-	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode, filp);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 	nfs_file_set_open_context(filp, ctx);
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 89a77950e0b0..0efba77789b9 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, struct file *filp)
 	parent = dget_parent(dentry);
 	dir = d_inode(parent);
 
-	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode, filp);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index b7df3ef84fc3..422ed5ac4efe 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3792,7 +3792,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 	struct nfs4_state *state;
 	int status = 0;
 
-	ctx = alloc_nfs_open_context(dentry, FMODE_READ);
+	ctx = alloc_nfs_open_context(dentry, FMODE_READ, NULL);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index bf8a713c45b4..0adb02c4744d 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -70,6 +70,7 @@ struct nfs_lock_context {
 struct nfs4_state;
 struct nfs_open_context {
 	struct nfs_lock_context lock_context;
+	fl_owner_t flock_owner;
 	struct dentry *dentry;
 	struct rpc_cred *cred;
 	struct nfs4_state *state;
@@ -357,7 +358,7 @@ extern void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr,
 extern struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx);
 extern void put_nfs_open_context(struct nfs_open_context *ctx);
 extern struct nfs_open_context *nfs_find_open_context(struct inode *inode, struct rpc_cred *cred, fmode_t mode);
-extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode);
+extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode, struct file *filp);
 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);
-- 
2.10.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 1/5] NFS: remove l_pid field from nfs_lockowner

[Jeff Layton]

On Wed, 2016-10-12 at 13:39 +1100, NeilBrown wrote:
> this field is not used in any important way and probably should
> have been removed by
> 
> Commit: 8003d3c4aaa5 ("nfs4: treat lock owners as opaque values")
> 
> which removed the pid argument from nfs4_get_lock_state.
> 
> Except in unusual and uninteresting cases, two threads with the same
> ->tgid will have the same ->files pointer, so keeping them both
> for comparison brings no benefit.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  fs/nfs/inode.c         |    3 ---
>  fs/nfs/nfs4proc.c      |    1 -
>  fs/nfs/pagelist.c      |    3 +--
>  fs/nfs/write.c         |    3 +--
>  include/linux/nfs_fs.h |    1 -
>  5 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index bf4ec5ecc97e..1752fc7c0024 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -703,7 +703,6 @@ static void nfs_init_lock_context(struct nfs_lock_context *l_ctx)
>  {
>  	atomic_set(&l_ctx->count, 1);
>  	l_ctx->lockowner.l_owner = current->files;
> -	l_ctx->lockowner.l_pid = current->tgid;
>  	INIT_LIST_HEAD(&l_ctx->list);
>  	atomic_set(&l_ctx->io_count, 0);
>  }
> @@ -716,8 +715,6 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context
>  	do {
>  		if (pos->lockowner.l_owner != current->files)
>  			continue;
> -		if (pos->lockowner.l_pid != current->tgid)
> -			continue;
>  		atomic_inc(&pos->count);
>  		return pos;
>  	} while ((pos = list_entry(pos->list.next, typeof(*pos), list)) != head);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 0e327528a3ce..b7df3ef84fc3 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2766,7 +2766,6 @@ static int _nfs4_do_setattr(struct inode *inode,
>  	} else if (truncate && state != NULL) {
>  		struct nfs_lockowner lockowner = {
>  			.l_owner = current->files,
> -			.l_pid = current->tgid,
>  		};
>  		if (!nfs4_valid_open_stateid(state))
>  			return -EBADF;
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 965db474f4b0..161f8b13bbaa 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -867,8 +867,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
> -		&& l1->lockowner.l_pid == l2->lockowner.l_pid;
> +	return l1->lockowner.l_owner == l2->lockowner.l_owner;
>  }
>  
>  /**
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 53211838f72a..4d5897e6d6cb 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1151,8 +1151,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
> -				|| l_ctx->lockowner.l_pid != current->tgid;
> +			do_flush |= l_ctx->lockowner.l_owner != current->files;
>  		}
>  		nfs_release_request(req);
>  		if (!do_flush)
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index 810124b33327..bf8a713c45b4 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -57,7 +57,6 @@ struct nfs_access_entry {
>  
>  struct nfs_lockowner {
>  	fl_owner_t l_owner;
> -	pid_t l_pid;
>  };
>  
>  struct nfs_lock_context {
> 
> 

Looks ok. For my own knowledge though, in what situations would you have
the same files pointer but a different tgid?

Acked-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/5 - version 2] NFSv4: add flock_owner to open context

[Jeff Layton]

On Wed, 2016-10-12 at 14:33 +1100, NeilBrown wrote:
> An open file description (struct file) in a given process can be
> associated with two different lock owners.
> 
> It can have a Posix lock owner which will be different in each process
> that has a fd on the file.
> It can have a Flock owner which will be the same in all processes.
> 
> When searching for a lock stateid to use, we need to consider both of these
> owners
> 
> So add a new "flock_owner" to the "nfs_open_context" (of which there
> is one for each open file description).
> 
> This flock_owner does not need to be reference-counted as there is a
> 1-1 relation between 'struct file' and nfs open contexts,
> and it will never be part of a list of contexts.  So there is no need
> for a 'flock_context' - just the owner is enough.
> 
> The io_count included in the (Posix) lock_context provides no
> guarantee that all read-aheads that could use the state have
> completed, so not supporting it for flock locks in not a serious
> problem.  Synchronization between flock and read-ahead can be added
> later if needed.
> 
> When creating an open_context for a non-openning create call, we don't have
> a 'struct file' to pass in, so the lock context gets initialized with
> a NULL owner, but this will never be used.
> 
> The flock_owner is not used at all in this patch, that will come later.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  fs/nfs/dir.c           | 6 +++---
>  fs/nfs/inode.c         | 7 +++++--
>  fs/nfs/nfs4file.c      | 2 +-
>  fs/nfs/nfs4proc.c      | 2 +-
>  include/linux/nfs_fs.h | 3 ++-
>  5 files changed, 12 insertions(+), 8 deletions(-)
> 
> Sorry, two little errors in previous version....
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 06e0bf092ba9..d5b87b28010d 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1453,9 +1453,9 @@ static fmode_t flags_to_mode(int flags)
>  	return res;
>  }
>  
> -static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags)
> +static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags, struct file *filp)
>  {
> -	return alloc_nfs_open_context(dentry, flags_to_mode(open_flags));
> +	return alloc_nfs_open_context(dentry, flags_to_mode(open_flags), filp);
>  }
>  
>  static int do_open(struct inode *inode, struct file *filp)
> @@ -1540,7 +1540,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>  			return finish_no_open(file, dentry);
>  	}
>  
> -	ctx = create_nfs_open_context(dentry, open_flags);
> +	ctx = create_nfs_open_context(dentry, open_flags, file);
>  	err = PTR_ERR(ctx);
>  	if (IS_ERR(ctx))
>  		goto out;
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 1752fc7c0024..2138027eaba9 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -796,7 +796,9 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync)
>  }
>  EXPORT_SYMBOL_GPL(nfs_close_context);
>  
> -struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode)
> +struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry,
> +						fmode_t f_mode,
> +						struct file *filp)
>  {
>  	struct nfs_open_context *ctx;
>  	struct rpc_cred *cred = rpc_lookup_cred();
> @@ -815,6 +817,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;
> +	ctx->flock_owner = (fl_owner_t)filp;
>  	nfs_init_lock_context(&ctx->lock_context);
>  	ctx->lock_context.open_context = ctx;
>  	INIT_LIST_HEAD(&ctx->list);
> @@ -939,7 +942,7 @@ int nfs_open(struct inode *inode, struct file *filp)
>  {
>  	struct nfs_open_context *ctx;
>  
> -	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
> +	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode, filp);
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
>  	nfs_file_set_open_context(filp, ctx);
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index 89a77950e0b0..0efba77789b9 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, struct file *filp)
>  	parent = dget_parent(dentry);
>  	dir = d_inode(parent);
>  
> -	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
> +	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode, filp);
>  	err = PTR_ERR(ctx);
>  	if (IS_ERR(ctx))
>  		goto out;
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index b7df3ef84fc3..422ed5ac4efe 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3792,7 +3792,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
>  	struct nfs4_state *state;
>  	int status = 0;
>  
> -	ctx = alloc_nfs_open_context(dentry, FMODE_READ);
> +	ctx = alloc_nfs_open_context(dentry, FMODE_READ, NULL);
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
>  
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index bf8a713c45b4..0adb02c4744d 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -70,6 +70,7 @@ struct nfs_lock_context {
>  struct nfs4_state;
>  struct nfs_open_context {
>  	struct nfs_lock_context lock_context;
> +	fl_owner_t flock_owner;
>  	struct dentry *dentry;
>  	struct rpc_cred *cred;
>  	struct nfs4_state *state;
> @@ -357,7 +358,7 @@ extern void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr,
>  extern struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx);
>  extern void put_nfs_open_context(struct nfs_open_context *ctx);
>  extern struct nfs_open_context *nfs_find_open_context(struct inode *inode, struct rpc_cred *cred, fmode_t mode);
> -extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode);
> +extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode, struct file *filp);
>  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);

Acked-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/5] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner

[Jeff Layton]

On Wed, 2016-10-12 at 13:39 +1100, NeilBrown wrote:
> The only time that a lock_context is not available is in setattr.
> In this case, we want to find a lock context relevant to the process if there is one.
> The fallback can easily be handled at a lower level.
> 
> So change nfs4_select_rw_stateid to take a lock_context, passing NULL from _nfs4_do_setattr.
> nfs4_copy_lock_state() also now takes a lock_context, and falls back to searching
> for "owner == current->files" if not lock_context is given.
> 
> Note that nfs4_set_rw_stateid is *always* passed a non-NULL l_ctx, so the
> fact that we remove the NULL test there does not change correctness.
> 
> This change is preparation for correctly support flock stateids.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  fs/nfs/nfs4_fs.h   |    2 +-
>  fs/nfs/nfs4proc.c  |   11 ++---------
>  fs/nfs/nfs4state.c |   19 +++++++++++--------
>  3 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 9bf64eacba5b..3f0e459f2499 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -445,7 +445,7 @@ extern void nfs41_handle_server_scope(struct nfs_client *,
>  extern void nfs4_put_lock_state(struct nfs4_lock_state *lsp);
>  extern int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl);
>  extern int nfs4_select_rw_stateid(struct nfs4_state *, fmode_t,
> -		const struct nfs_lockowner *, nfs4_stateid *,
> +		const struct nfs_lock_context *, nfs4_stateid *,
>  		struct rpc_cred **);
>  
>  extern struct nfs_seqid *nfs_alloc_seqid(struct nfs_seqid_counter *counter, gfp_t gfp_mask);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 422ed5ac4efe..6820c44aebe4 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2764,12 +2764,9 @@ static int _nfs4_do_setattr(struct inode *inode,
>  	if (nfs4_copy_delegation_stateid(inode, fmode, &arg->stateid, &delegation_cred)) {
>  		/* Use that stateid */
>  	} else if (truncate && state != NULL) {
> -		struct nfs_lockowner lockowner = {
> -			.l_owner = current->files,
> -		};
>  		if (!nfs4_valid_open_stateid(state))
>  			return -EBADF;
> -		if (nfs4_select_rw_stateid(state, FMODE_WRITE, &lockowner,
> +		if (nfs4_select_rw_stateid(state, FMODE_WRITE, NULL,
>  				&arg->stateid, &delegation_cred) == -EIO)
>  			return -EBADF;
>  	} else
> @@ -4362,11 +4359,7 @@ int nfs4_set_rw_stateid(nfs4_stateid *stateid,
>  		const struct nfs_lock_context *l_ctx,
>  		fmode_t fmode)
>  {
> -	const struct nfs_lockowner *lockowner = NULL;
> -
> -	if (l_ctx != NULL)
> -		lockowner = &l_ctx->lockowner;
> -	return nfs4_select_rw_stateid(ctx->state, fmode, lockowner, stateid, NULL);
> +	return nfs4_select_rw_stateid(ctx->state, fmode, l_ctx, stateid, NULL);
>  }
>  EXPORT_SYMBOL_GPL(nfs4_set_rw_stateid);
>  
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index cada00aa5096..94a6631e7938 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -939,20 +939,23 @@ int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl)
>  
>  static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
>  		struct nfs4_state *state,
> -		const struct nfs_lockowner *lockowner)
> +		const struct nfs_lock_context *l_ctx)
>  {
> +	/*
> +	 * If l_ctx is NULL, then this request comes from setattr
> +	 * and we can choose a lock context relevant for the current process
> +	 */
>  	struct nfs4_lock_state *lsp;
>  	fl_owner_t fl_owner;
>  	int ret = -ENOENT;
>  
> -
> -	if (lockowner == NULL)
> -		goto out;
> -
>  	if (test_bit(LK_STATE_IN_USE, &state->flags) == 0)
>  		goto out;
>  
> -	fl_owner = lockowner->l_owner;
> +	if (l_ctx == NULL)
> +		fl_owner = current->files;
> +	else
> +		fl_owner = l_ctx->lockowner.l_owner;


This I'm less sure of. Suppose we have only a flock lock on a file and
then truncate it. This is going to miss finding that stateid, no?

I wonder if we need to look harder at the state to determine which owner to use in this case?


>  	spin_lock(&state->state_lock);
>  	lsp = __nfs4_find_lock_state(state, fl_owner);
>  	if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags))
> @@ -986,14 +989,14 @@ static void nfs4_copy_open_stateid(nfs4_stateid *dst, struct nfs4_state *state)
>   * requests.
>   */
>  int nfs4_select_rw_stateid(struct nfs4_state *state,
> -		fmode_t fmode, const struct nfs_lockowner *lockowner,
> +		fmode_t fmode, const struct nfs_lock_context *l_ctx,
>  		nfs4_stateid *dst, struct rpc_cred **cred)
>  {
>  	int ret;
>  
>  	if (cred != NULL)
>  		*cred = NULL;
> -	ret = nfs4_copy_lock_stateid(dst, state, lockowner);
> +	ret = nfs4_copy_lock_stateid(dst, state, l_ctx);
>  	if (ret == -EIO)
>  		/* A lost lock - don't even consider delegations */
>  		goto out;
> 
> 

Also, as a side note: There were some changes around the NFS file
locking code that went into the CB_NOTIFY_LOCK patches. Those are in
linux-next now, and I _think_ Anna is going to merge them for v4.9. I
don't see any obvious conflicts here, but you may want to ensure that
you're basing this on top of linux-next to minimize them.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/5] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner

[Anna Schumaker]

On 10/12/2016 08:33 AM, Jeff Layton wrote:
> On Wed, 2016-10-12 at 13:39 +1100, NeilBrown wrote:
>> The only time that a lock_context is not available is in setattr.
>> In this case, we want to find a lock context relevant to the process if there is one.
>> The fallback can easily be handled at a lower level.
>>
>> So change nfs4_select_rw_stateid to take a lock_context, passing NULL from _nfs4_do_setattr.
>> nfs4_copy_lock_state() also now takes a lock_context, and falls back to searching
>> for "owner == current->files" if not lock_context is given.
>>
>> Note that nfs4_set_rw_stateid is *always* passed a non-NULL l_ctx, so the
>> fact that we remove the NULL test there does not change correctness.
>>
>> This change is preparation for correctly support flock stateids.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  fs/nfs/nfs4_fs.h   |    2 +-
>>  fs/nfs/nfs4proc.c  |   11 ++---------
>>  fs/nfs/nfs4state.c |   19 +++++++++++--------
>>  3 files changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>> index 9bf64eacba5b..3f0e459f2499 100644
>> --- a/fs/nfs/nfs4_fs.h
>> +++ b/fs/nfs/nfs4_fs.h
>> @@ -445,7 +445,7 @@ extern void nfs41_handle_server_scope(struct nfs_client *,
>>  extern void nfs4_put_lock_state(struct nfs4_lock_state *lsp);
>>  extern int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl);
>>  extern int nfs4_select_rw_stateid(struct nfs4_state *, fmode_t,
>> -		const struct nfs_lockowner *, nfs4_stateid *,
>> +		const struct nfs_lock_context *, nfs4_stateid *,
>>  		struct rpc_cred **);
>>  
>>  extern struct nfs_seqid *nfs_alloc_seqid(struct nfs_seqid_counter *counter, gfp_t gfp_mask);
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 422ed5ac4efe..6820c44aebe4 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -2764,12 +2764,9 @@ static int _nfs4_do_setattr(struct inode *inode,
>>  	if (nfs4_copy_delegation_stateid(inode, fmode, &arg->stateid, &delegation_cred)) {
>>  		/* Use that stateid */
>>  	} else if (truncate && state != NULL) {
>> -		struct nfs_lockowner lockowner = {
>> -			.l_owner = current->files,
>> -		};
>>  		if (!nfs4_valid_open_stateid(state))
>>  			return -EBADF;
>> -		if (nfs4_select_rw_stateid(state, FMODE_WRITE, &lockowner,
>> +		if (nfs4_select_rw_stateid(state, FMODE_WRITE, NULL,
>>  				&arg->stateid, &delegation_cred) == -EIO)
>>  			return -EBADF;
>>  	} else
>> @@ -4362,11 +4359,7 @@ int nfs4_set_rw_stateid(nfs4_stateid *stateid,
>>  		const struct nfs_lock_context *l_ctx,
>>  		fmode_t fmode)
>>  {
>> -	const struct nfs_lockowner *lockowner = NULL;
>> -
>> -	if (l_ctx != NULL)
>> -		lockowner = &l_ctx->lockowner;
>> -	return nfs4_select_rw_stateid(ctx->state, fmode, lockowner, stateid, NULL);
>> +	return nfs4_select_rw_stateid(ctx->state, fmode, l_ctx, stateid, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(nfs4_set_rw_stateid);
>>  
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index cada00aa5096..94a6631e7938 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -939,20 +939,23 @@ int nfs4_set_lock_state(struct nfs4_state *state, struct file_lock *fl)
>>  
>>  static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
>>  		struct nfs4_state *state,
>> -		const struct nfs_lockowner *lockowner)
>> +		const struct nfs_lock_context *l_ctx)
>>  {
>> +	/*
>> +	 * If l_ctx is NULL, then this request comes from setattr
>> +	 * and we can choose a lock context relevant for the current process
>> +	 */
>>  	struct nfs4_lock_state *lsp;
>>  	fl_owner_t fl_owner;
>>  	int ret = -ENOENT;
>>  
>> -
>> -	if (lockowner == NULL)
>> -		goto out;
>> -
>>  	if (test_bit(LK_STATE_IN_USE, &state->flags) == 0)
>>  		goto out;
>>  
>> -	fl_owner = lockowner->l_owner;
>> +	if (l_ctx == NULL)
>> +		fl_owner = current->files;
>> +	else
>> +		fl_owner = l_ctx->lockowner.l_owner;
> 
> 
> This I'm less sure of. Suppose we have only a flock lock on a file and
> then truncate it. This is going to miss finding that stateid, no?
> 
> I wonder if we need to look harder at the state to determine which owner to use in this case?
> 
> 
>>  	spin_lock(&state->state_lock);
>>  	lsp = __nfs4_find_lock_state(state, fl_owner);
>>  	if (lsp && test_bit(NFS_LOCK_LOST, &lsp->ls_flags))
>> @@ -986,14 +989,14 @@ static void nfs4_copy_open_stateid(nfs4_stateid *dst, struct nfs4_state *state)
>>   * requests.
>>   */
>>  int nfs4_select_rw_stateid(struct nfs4_state *state,
>> -		fmode_t fmode, const struct nfs_lockowner *lockowner,
>> +		fmode_t fmode, const struct nfs_lock_context *l_ctx,
>>  		nfs4_stateid *dst, struct rpc_cred **cred)
>>  {
>>  	int ret;
>>  
>>  	if (cred != NULL)
>>  		*cred = NULL;
>> -	ret = nfs4_copy_lock_stateid(dst, state, lockowner);
>> +	ret = nfs4_copy_lock_stateid(dst, state, l_ctx);
>>  	if (ret == -EIO)
>>  		/* A lost lock - don't even consider delegations */
>>  		goto out;
>>
>>
> 
> Also, as a side note: There were some changes around the NFS file
> locking code that went into the CB_NOTIFY_LOCK patches. Those are in
> linux-next now, and I _think_ Anna is going to merge them for v4.9. I
> don't see any obvious conflicts here, but you may want to ensure that
> you're basing this on top of linux-next to minimize them.

Yep, they'll be in 4.9.  Updating against those changes wouldn't hurt :)

Anna

> 

Re: [PATCH 1/5] NFS: remove l_pid field from nfs_lockowner

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On Wed, Oct 12 2016, Jeff Layton wrote:

>
> Looks ok. For my own knowledge though, in what situations would you have
> the same files pointer but a different tgid?

If you call clone(2) passing CLONE_FILES (so get the same files pointer)
but not CLONE_THREAD (so different thread-group).
Definitely possible, probably not useful.

>
> Acked-by: Jeff Layton <jlayton@redhat.com>

Thanks,

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 3/5] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On Thu, Oct 13 2016, Anna Schumaker wrote:
>> 
>> Also, as a side note: There were some changes around the NFS file
>> locking code that went into the CB_NOTIFY_LOCK patches. Those are in
>> linux-next now, and I _think_ Anna is going to merge them for v4.9. I
>> don't see any obvious conflicts here, but you may want to ensure that
>> you're basing this on top of linux-next to minimize them.
>
> Yep, they'll be in 4.9.  Updating against those changes wouldn't hurt :)

Ok....
I note that
  git://git.linux-nfs.org/projects/anna/linux-nfs.git#linux-next

isn't in
  git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

Should it be?

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 3/5] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

On Wed, Oct 12 2016, Jeff Layton wrote:

>> -	fl_owner = lockowner->l_owner;
>> +	if (l_ctx == NULL)
>> +		fl_owner = current->files;
>> +	else
>> +		fl_owner = l_ctx->lockowner.l_owner;
>
>
> This I'm less sure of. Suppose we have only a flock lock on a file and
> then truncate it. This is going to miss finding that stateid, no?
>
> I wonder if we need to look harder at the state to determine which owner to use in this case?

I didn't think this was possible with the current API, but I had
forgotten about (or never knew about) ATTR_FILE.
setattr does have access to the file, so it can find all the nfs state
info needed.
And when I modify the patch series to make use of that, it removes some
ugly bits :-)

I'll repost after a little testing.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 3/5] NFSv4: change nfs4_select_rw_stateid to take a lock_context inplace of lock_owner

[Jeff Layton]

On Thu, 2016-10-13 at 15:04 +1100, NeilBrown wrote:
> On Thu, Oct 13 2016, Anna Schumaker wrote:
> > 
> > > 
> > > 
> > > Also, as a side note: There were some changes around the NFS file
> > > locking code that went into the CB_NOTIFY_LOCK patches. Those are in
> > > linux-next now, and I _think_ Anna is going to merge them for v4.9. I
> > > don't see any obvious conflicts here, but you may want to ensure that
> > > you're basing this on top of linux-next to minimize them.
> > 
> > Yep, they'll be in 4.9.  Updating against those changes wouldn't hurt :)
> 
> Ok....
> I note that
>   git://git.linux-nfs.org/projects/anna/linux-nfs.git#linux-next
> 
> isn't in
>   git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> 
> Should it be?
> 
> NeilBrown

Yes, it seems like that should be going into -next.

Anna, do you and Trond have something worked out where the patches in
your pull requests are going into -next through Trond's tree?
-- 
Jeff Layton <jlayton@redhat.com>