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

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

[NeilBrown]

Thanks to prompting from Jeff I discovered that I could
access the correct open-state when ftrunctate() is being used.
That improves the setattr handling, removing some special cases.

This series is based on the linux-next branch of
	git://git.linux-nfs.org/projects/anna/linux-nfs.git

NeilBrown

---

NeilBrown (6):
      NFS: remove l_pid field from nfs_lockowner
      NFSv4: add flock_owner to open context
      NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
      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      |   44 +++++++++++++++++++-------------------------
 fs/nfs/nfs4state.c     |   25 ++++++++++++++-----------
 fs/nfs/pagelist.c      |    3 +--
 fs/nfs/write.c         |    3 +--
 include/linux/nfs_fs.h |   10 +++-------
 9 files changed, 50 insertions(+), 59 deletions(-)

--
Signature

[PATCH 1/6] 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.

Acked-by: Jeff Layton <jlayton@redhat.com>
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 ad917bd72b38..3c164f9a3983 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2928,7 +2928,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/6] 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.

Acked-by: Jeff Layton <jlayton@redhat.com>
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 5f1af4cd1a33..f1ecfcc632eb 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1467,9 +1467,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)
@@ -1554,7 +1554,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 3c164f9a3983..3d85ab7b56bf 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3957,7 +3957,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);

[PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.

[NeilBrown]

The open_context can always lead directly to the state, and is always easily
available, so this is a straightforward change.
Doing this makes more information available to _nfs4_do_setattr() for use
in the next patch.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/nfs4proc.c |   28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 3d85ab7b56bf..950b25413bb4 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -94,7 +94,7 @@ static int nfs4_proc_getattr(struct nfs_server *, struct nfs_fh *, struct nfs_fa
 static int _nfs4_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle, struct nfs_fattr *fattr, struct nfs4_label *label);
 static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 			    struct nfs_fattr *fattr, struct iattr *sattr,
-			    struct nfs4_state *state, struct nfs4_label *ilabel,
+			    struct nfs_open_context *ctx, struct nfs4_label *ilabel,
 			    struct nfs4_label *olabel);
 #ifdef CONFIG_NFS_V4_1
 static int nfs41_test_stateid(struct nfs_server *, nfs4_stateid *,
@@ -2807,7 +2807,7 @@ static int _nfs4_do_open(struct inode *dir,
 			nfs_fattr_init(opendata->o_res.f_attr);
 			status = nfs4_do_setattr(state->inode, cred,
 					opendata->o_res.f_attr, sattr,
-					state, label, olabel);
+					ctx, label, olabel);
 			if (status == 0) {
 				nfs_setattr_update_inode(state->inode, sattr,
 						opendata->o_res.f_attr);
@@ -2902,7 +2902,7 @@ static int _nfs4_do_setattr(struct inode *inode,
 			    struct nfs_setattrargs *arg,
 			    struct nfs_setattrres *res,
 			    struct rpc_cred *cred,
-			    struct nfs4_state *state)
+			    struct nfs_open_context *ctx)
 {
 	struct nfs_server *server = NFS_SERVER(inode);
         struct rpc_message msg = {
@@ -2925,13 +2925,13 @@ 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) {
+	} else if (truncate && ctx != NULL) {
 		struct nfs_lockowner lockowner = {
 			.l_owner = current->files,
 		};
-		if (!nfs4_valid_open_stateid(state))
+		if (!nfs4_valid_open_stateid(ctx->state))
 			return -EBADF;
-		if (nfs4_select_rw_stateid(state, FMODE_WRITE, &lockowner,
+		if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, &lockowner,
 				&arg->stateid, &delegation_cred) == -EIO)
 			return -EBADF;
 	} else
@@ -2942,7 +2942,7 @@ static int _nfs4_do_setattr(struct inode *inode,
 	status = nfs4_call_sync(server->client, server, &msg, &arg->seq_args, &res->seq_res, 1);
 
 	put_rpccred(delegation_cred);
-	if (status == 0 && state != NULL)
+	if (status == 0 && ctx != NULL)
 		renew_lease(server, timestamp);
 	trace_nfs4_setattr(inode, &arg->stateid, status);
 	return status;
@@ -2950,10 +2950,11 @@ static int _nfs4_do_setattr(struct inode *inode,
 
 static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 			   struct nfs_fattr *fattr, struct iattr *sattr,
-			   struct nfs4_state *state, struct nfs4_label *ilabel,
+			   struct nfs_open_context *ctx, struct nfs4_label *ilabel,
 			   struct nfs4_label *olabel)
 {
 	struct nfs_server *server = NFS_SERVER(inode);
+	struct nfs4_state *state = ctx ? ctx->state : NULL;
         struct nfs_setattrargs  arg = {
                 .fh             = NFS_FH(inode),
                 .iap            = sattr,
@@ -2978,7 +2979,7 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 		arg.bitmask = nfs4_bitmask(server, olabel);
 
 	do {
-		err = _nfs4_do_setattr(inode, &arg, &res, cred, state);
+		err = _nfs4_do_setattr(inode, &arg, &res, cred, ctx);
 		switch (err) {
 		case -NFS4ERR_OPENMODE:
 			if (!(sattr->ia_valid & ATTR_SIZE)) {
@@ -3673,7 +3674,7 @@ nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
 {
 	struct inode *inode = d_inode(dentry);
 	struct rpc_cred *cred = NULL;
-	struct nfs4_state *state = NULL;
+	struct nfs_open_context *ctx = NULL;
 	struct nfs4_label *label = NULL;
 	int status;
 
@@ -3694,20 +3695,17 @@ nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
 
 	/* Search for an existing open(O_WRITE) file */
 	if (sattr->ia_valid & ATTR_FILE) {
-		struct nfs_open_context *ctx;
 
 		ctx = nfs_file_open_context(sattr->ia_file);
-		if (ctx) {
+		if (ctx)
 			cred = ctx->cred;
-			state = ctx->state;
-		}
 	}
 
 	label = nfs4_label_alloc(NFS_SERVER(inode), GFP_KERNEL);
 	if (IS_ERR(label))
 		return PTR_ERR(label);
 
-	status = nfs4_do_setattr(inode, cred, fattr, sattr, state, NULL, label);
+	status = nfs4_do_setattr(inode, cred, fattr, sattr, ctx, NULL, label);
 	if (status == 0) {
 		nfs_setattr_update_inode(inode, sattr, fattr);
 		nfs_setsecurity(inode, fattr, label);

[PATCH 4/6] 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 immediately available is in
setattr, and now that it has an open_context, it can easily find one
with nfs_get_lock_context.
This removes the need for the on-stack nfs_lockowner.

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  |   15 ++++++---------
 fs/nfs/nfs4state.c |   11 +++++------
 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 9b3a82abab07..7784b79915e1 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -457,7 +457,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 950b25413bb4..3c2f11189794 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2926,12 +2926,13 @@ 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 && ctx != NULL) {
-		struct nfs_lockowner lockowner = {
-			.l_owner = current->files,
-		};
+		struct nfs_lock_context *l_ctx;
 		if (!nfs4_valid_open_stateid(ctx->state))
 			return -EBADF;
-		if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, &lockowner,
+		l_ctx = nfs_get_lock_context(ctx);
+		if (IS_ERR(l_ctx))
+			return PTR_ERR(l_ctx);
+		if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, l_ctx,
 				&arg->stateid, &delegation_cred) == -EIO)
 			return -EBADF;
 	} else
@@ -4525,11 +4526,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 5f4281ec5f72..f25eee8202bf 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -939,20 +939,19 @@ 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)
 {
 	struct nfs4_lock_state *lsp;
 	fl_owner_t fl_owner;
 	int ret = -ENOENT;
 
-
-	if (lockowner == NULL)
+	if (l_ctx == NULL)
 		goto out;
 
 	if (test_bit(LK_STATE_IN_USE, &state->flags) == 0)
 		goto out;
 
-	fl_owner = lockowner->l_owner;
+	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,7 +985,7 @@ 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;
@@ -995,7 +994,7 @@ int nfs4_select_rw_stateid(struct nfs4_state *state,
 		return -EIO;
 	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 5/6] 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 |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index f25eee8202bf..ed39ee164f5f 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) {
@@ -942,7 +944,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
 		const struct nfs_lock_context *l_ctx)
 {
 	struct nfs4_lock_state *lsp;
-	fl_owner_t fl_owner;
+	fl_owner_t fl_owner, fl_flock_owner;
 	int ret = -ENOENT;
 
 	if (l_ctx == NULL)
@@ -952,8 +954,10 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
 		goto out;
 
 	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 6/6] 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 2138027eaba9..72e992f0f049 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 ed39ee164f5f..da2e6b791316 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -953,7 +953,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
 	if (test_bit(LK_STATE_IN_USE, &state->flags) == 0)
 		goto out;
 
-	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 0adb02c4744d..db1002abc95e 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;
 };
 

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

[Jeff Layton]

On Thu, 2016-10-13 at 15:26 +1100, NeilBrown wrote:
> 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 |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index f25eee8202bf..ed39ee164f5f 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;

Ok, so we end up getting whatever is first on the list here. That's
certainly fine when there are either flock/OFD locks or traditional
POSIX locks in use.

When there are both in use though, then things may be less predictable.
That said, mixing flock/OFD and POSIX locks on the same fds from the
same process is not a great idea in general, and I have a hard time
coming up with a valid use-case there.

So, I don't see that as a real problem, but it may be worth explaining
that rationale in the comment block above this function in case we need
to revisit it later.

> @@ -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) {
> @@ -942,7 +944,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
>  		const struct nfs_lock_context *l_ctx)
>  {
>  	struct nfs4_lock_state *lsp;
> -	fl_owner_t fl_owner;
> +	fl_owner_t fl_owner, fl_flock_owner;
>  	int ret = -ENOENT;
>  
>  	if (l_ctx == NULL)
> @@ -952,8 +954,10 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
>  		goto out;
>  
>  	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) {
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2

[Jeff Layton]

On Thu, 2016-10-13 at 15:26 +1100, NeilBrown wrote:
> Thanks to prompting from Jeff I discovered that I could
> access the correct open-state when ftrunctate() is being used.
> That improves the setattr handling, removing some special cases.
> 
> This series is based on the linux-next branch of
> 	git://git.linux-nfs.org/projects/anna/linux-nfs.git
> 
> NeilBrown
> 
> ---
> 
> NeilBrown (6):
>       NFS: remove l_pid field from nfs_lockowner
>       NFSv4: add flock_owner to open context
>       NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
>       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      |   44 +++++++++++++++++++-------------------------
>  fs/nfs/nfs4state.c     |   25 ++++++++++++++-----------
>  fs/nfs/pagelist.c      |    3 +--
>  fs/nfs/write.c         |    3 +--
>  include/linux/nfs_fs.h |   10 +++-------
>  9 files changed, 50 insertions(+), 59 deletions(-)
> 
> --
> Signature
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

This all looks good to me aside from some minor documentation nits.
Kudos too on doing this with a net removal of code. :)

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

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

[NeilBrown]

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

On Fri, Oct 14 2016, Jeff Layton wrote:

> On Thu, 2016-10-13 at 15:26 +1100, NeilBrown wrote:
>> 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 |   14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index f25eee8202bf..ed39ee164f5f 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;
>
> Ok, so we end up getting whatever is first on the list here. That's
> certainly fine when there are either flock/OFD locks or traditional
> POSIX locks in use.
>
> When there are both in use though, then things may be less predictable.
> That said, mixing flock/OFD and POSIX locks on the same fds from the
> same process is not a great idea in general, and I have a hard time
> coming up with a valid use-case there.

Using two types of locks in the one application would be ... unusual.
I wouldn't want to spend much of addressing any issues, but not being
predictable isn't good.  Intermittent problems are so hard to debug.

We should probably make sure it consistently chooses on or the other.
As flock locks are always whole-file, it is always safe to choose the
flock lock over the posix lock as you can be sure the IO is covered by
the lock. OFD locks make that a little be less of a clear choice.

On the other hand, NFS locks were originally only Posix locks and flock
locks were only supported much later.  So for historical consistency we
should probably choose the Posix stateid preferentially.

I find the second argument more convincing.   Here is the updated patch.

Thanks,
NeilBrown

From: NeilBrown <neilb@suse.com>
Subject: [PATCH] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid
 if there is one

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 | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index f25eee8202bf..bd29d4360660 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -797,19 +797,33 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
 
 /*
  * Search the state->lock_states for an existing lock_owner
- * that is compatible with current->files
+ * that is compatible with either of the given owners.
+ * If the second is non-zero, then the first refers to a Posix-lock
+ * owner (current->files) and the second refers to a flock/OFD
+ * owner (struct file*).  In that case, prefer a match for the first
+ * owner.
+ * If both sorts of locks are held on the one file we cannot know
+ * which stateid was intended to be used, so a "correct" choice cannot
+ * be made.  Failing that, a "consistent" choice is preferable.  The
+ * consistent choice we make is to prefer the first owner, that of a
+ * Posix lock.
  */
 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;
+	struct nfs4_lock_state *pos, *ret = NULL;
 	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 == fl_owner) {
+			ret = pos;
+			break;
+		}
+		if (pos->ls_owner == fl_owner2)
+			ret = pos;
 	}
-	return NULL;
+	if (ret)
+		atomic_inc(&ret->ls_count);
+	return ret;
 }
 
 /*
@@ -857,7 +871,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) {
@@ -942,7 +956,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
 		const struct nfs_lock_context *l_ctx)
 {
 	struct nfs4_lock_state *lsp;
-	fl_owner_t fl_owner;
+	fl_owner_t fl_owner, fl_flock_owner;
 	int ret = -ENOENT;
 
 	if (l_ctx == NULL)
@@ -952,8 +966,10 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
 		goto out;
 
 	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) {
-- 
2.10.0


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

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

[Jeff Layton]

On Fri, 2016-10-14 at 11:22 +1100, NeilBrown wrote:
> On Fri, Oct 14 2016, Jeff Layton wrote:
> 
> > 
> > On Thu, 2016-10-13 at 15:26 +1100, NeilBrown wrote:
> > > 
> > > 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 |   14 +++++++++-----
> > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > > index f25eee8202bf..ed39ee164f5f 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;
> > 
> > Ok, so we end up getting whatever is first on the list here. That's
> > certainly fine when there are either flock/OFD locks or traditional
> > POSIX locks in use.
> > 
> > When there are both in use though, then things may be less predictable.
> > That said, mixing flock/OFD and POSIX locks on the same fds from the
> > same process is not a great idea in general, and I have a hard time
> > coming up with a valid use-case there.
> 
> Using two types of locks in the one application would be ... unusual.
> I wouldn't want to spend much of addressing any issues, but not being
> predictable isn't good.  Intermittent problems are so hard to debug.
> 
> We should probably make sure it consistently chooses on or the other.
> As flock locks are always whole-file, it is always safe to choose the
> flock lock over the posix lock as you can be sure the IO is covered by
> the lock. OFD locks make that a little be less of a clear choice.
> 
> On the other hand, NFS locks were originally only Posix locks and flock
> locks were only supported much later.  So for historical consistency we
> should probably choose the Posix stateid preferentially.
> 
> I find the second argument more convincing.   Here is the updated patch.
> 
> Thanks,
> NeilBrown
> 
> From: NeilBrown <neilb@suse.com>
> Subject: [PATCH] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid
>  if there is one
> 
> 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 | 38 +++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index f25eee8202bf..bd29d4360660 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -797,19 +797,33 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
>  
>  /*
>   * Search the state->lock_states for an existing lock_owner
> - * that is compatible with current->files
> + * that is compatible with either of the given owners.
> + * If the second is non-zero, then the first refers to a Posix-lock
> + * owner (current->files) and the second refers to a flock/OFD
> + * owner (struct file*).  In that case, prefer a match for the first
> + * owner.
> + * If both sorts of locks are held on the one file we cannot know
> + * which stateid was intended to be used, so a "correct" choice cannot
> + * be made.  Failing that, a "consistent" choice is preferable.  The
> + * consistent choice we make is to prefer the first owner, that of a
> + * Posix lock.
>   */
>  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;
> +	struct nfs4_lock_state *pos, *ret = NULL;
>  	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 == fl_owner) {
> +			ret = pos;
> +			break;
> +		}
> +		if (pos->ls_owner == fl_owner2)
> +			ret = pos;
>  	}
> -	return NULL;
> +	if (ret)
> +		atomic_inc(&ret->ls_count);
> +	return ret;
>  }
>  
>  /*
> @@ -857,7 +871,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) {
> @@ -942,7 +956,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
>  		const struct nfs_lock_context *l_ctx)
>  {
>  	struct nfs4_lock_state *lsp;
> -	fl_owner_t fl_owner;
> +	fl_owner_t fl_owner, fl_flock_owner;
>  	int ret = -ENOENT;
>  
>  	if (l_ctx == NULL)
> @@ -952,8 +966,10 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst,
>  		goto out;
>  
>  	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) {

Predictable behavior is even better there, and I agree that picking
POSIX locks over flock/OFD makes more sense for historical reasons.

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

Re: [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2

[NeilBrown]

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

On Fri, Oct 14 2016, Jeff Layton wrote:

>
> This all looks good to me aside from some minor documentation nits.
> Kudos too on doing this with a net removal of code. :)
>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>

Thanks.

The full series, with Reviewed-by added, is available as below if
a "git pull" is easier that plucking them out of emails

NeilBrown


The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:

  Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)

are available in the git repository at:

  git://neil.brown.name/linux tags/nfs-flock-fix

for you to fetch changes up to 077829bfd0e32131b49d87476b69a189e8e13ae8:

  NFS: discard nfs_lockowner structure. (2016-10-19 08:44:25 +1100)

----------------------------------------------------------------
Series to fix NFSv4 stateid used for flock locks

----------------------------------------------------------------
NeilBrown (6):
      NFS: remove l_pid field from nfs_lockowner
      NFSv4: add flock_owner to open context
      NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
      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      | 44 +++++++++++++++++++-------------------------
 fs/nfs/nfs4state.c     | 49 ++++++++++++++++++++++++++++++++-----------------
 fs/nfs/pagelist.c      |  3 +--
 fs/nfs/write.c         |  3 +--
 include/linux/nfs_fs.h | 10 +++-------
 9 files changed, 68 insertions(+), 65 deletions(-)

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

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

[NeilBrown]

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

On Thu, Oct 13 2016, NeilBrown wrote:

> The only time that a lock_context is not immediately available is in
> setattr, and now that it has an open_context, it can easily find one
> with nfs_get_lock_context.
> This removes the need for the on-stack nfs_lockowner.
>
> 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  |   15 ++++++---------
>  fs/nfs/nfs4state.c |   11 +++++------
>  3 files changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 9b3a82abab07..7784b79915e1 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -457,7 +457,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 950b25413bb4..3c2f11189794 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2926,12 +2926,13 @@ 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 && ctx != NULL) {
> -		struct nfs_lockowner lockowner = {
> -			.l_owner = current->files,
> -		};
> +		struct nfs_lock_context *l_ctx;
>  		if (!nfs4_valid_open_stateid(ctx->state))
>  			return -EBADF;
> -		if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, &lockowner,
> +		l_ctx = nfs_get_lock_context(ctx);
> +		if (IS_ERR(l_ctx))
> +			return PTR_ERR(l_ctx);
> +		if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, l_ctx,
>  				&arg->stateid, &delegation_cred) == -EIO)
>  			return -EBADF;

Oops...
I have an nfs_get_lock_context() here with no matching
nfs_put_lock_context();


I've merged:

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 3c2f11189794..be4e3d9de1eb 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2932,8 +2932,10 @@ static int _nfs4_do_setattr(struct inode *inode,
 		l_ctx = nfs_get_lock_context(ctx);
 		if (IS_ERR(l_ctx))
 			return PTR_ERR(l_ctx);
-		if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, l_ctx,
-				&arg->stateid, &delegation_cred) == -EIO)
+		status = nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, l_ctx,
+						&arg->stateid, &delegation_cred);
+		nfs_put_lock_context(l_ctx);
+		if (status == -EIO)
 			return -EBADF;
 	} else
 		nfs4_stateid_copy(&arg->stateid, &zero_stateid);

into that patch and pushed out a new

  git://neil.brown.name/linux tags/nfs-flock-fix

 (3f5546538d7c678b82d10ce6add29a6d92565f1e)


Please let me know if I should re-post the series.

Thanks,
NeilBrown

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

Re: [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.

[Benjamin Coddington]

On 13 Oct 2016, at 0:26, NeilBrown wrote:

> The open_context can always lead directly to the state, and is always 
> easily
> available, so this is a straightforward change.
> Doing this makes more information available to _nfs4_do_setattr() for 
> use
> in the next patch.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  fs/nfs/nfs4proc.c |   28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 3d85ab7b56bf..950b25413bb4 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -94,7 +94,7 @@ static int nfs4_proc_getattr(struct nfs_server *, 
> struct nfs_fh *, struct nfs_fa
>  static int _nfs4_proc_getattr(struct nfs_server *server, struct 
> nfs_fh *fhandle, struct nfs_fattr *fattr, struct nfs4_label *label);
>  static int nfs4_do_setattr(struct inode *inode, struct rpc_cred 
> *cred,
>  			    struct nfs_fattr *fattr, struct iattr *sattr,
> -			    struct nfs4_state *state, struct nfs4_label *ilabel,
> +			    struct nfs_open_context *ctx, struct nfs4_label *ilabel,
>  			    struct nfs4_label *olabel);
>  #ifdef CONFIG_NFS_V4_1
>  static int nfs41_test_stateid(struct nfs_server *, nfs4_stateid *,
> @@ -2807,7 +2807,7 @@ static int _nfs4_do_open(struct inode *dir,
>  			nfs_fattr_init(opendata->o_res.f_attr);
>  			status = nfs4_do_setattr(state->inode, cred,
>  					opendata->o_res.f_attr, sattr,
> -					state, label, olabel);
> +					ctx, label, olabel);
>  			if (status == 0) {
>  				nfs_setattr_update_inode(state->inode, sattr,
>  						opendata->o_res.f_attr);
> @@ -2902,7 +2902,7 @@ static int _nfs4_do_setattr(struct inode *inode,
>  			    struct nfs_setattrargs *arg,
>  			    struct nfs_setattrres *res,
>  			    struct rpc_cred *cred,
> -			    struct nfs4_state *state)
> +			    struct nfs_open_context *ctx)
>  {
>  	struct nfs_server *server = NFS_SERVER(inode);
>          struct rpc_message msg = {
> @@ -2925,13 +2925,13 @@ 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) {
> +	} else if (truncate && ctx != NULL) {
>  		struct nfs_lockowner lockowner = {
>  			.l_owner = current->files,
>  		};
> -		if (!nfs4_valid_open_stateid(state))
> +		if (!nfs4_valid_open_stateid(ctx->state))
>  			return -EBADF;
> -		if (nfs4_select_rw_stateid(state, FMODE_WRITE, &lockowner,
> +		if (nfs4_select_rw_stateid(ctx->state, FMODE_WRITE, &lockowner,
>  				&arg->stateid, &delegation_cred) == -EIO)
>  			return -EBADF;
>  	} else
> @@ -2942,7 +2942,7 @@ static int _nfs4_do_setattr(struct inode *inode,
>  	status = nfs4_call_sync(server->client, server, &msg, 
> &arg->seq_args, &res->seq_res, 1);
>
>  	put_rpccred(delegation_cred);
> -	if (status == 0 && state != NULL)
> +	if (status == 0 && ctx != NULL)
>  		renew_lease(server, timestamp);
>  	trace_nfs4_setattr(inode, &arg->stateid, status);
>  	return status;
> @@ -2950,10 +2950,11 @@ static int _nfs4_do_setattr(struct inode 
> *inode,
>
>  static int nfs4_do_setattr(struct inode *inode, struct rpc_cred 
> *cred,
>  			   struct nfs_fattr *fattr, struct iattr *sattr,
> -			   struct nfs4_state *state, struct nfs4_label *ilabel,
> +			   struct nfs_open_context *ctx, struct nfs4_label *ilabel,
>  			   struct nfs4_label *olabel)
>  {
>  	struct nfs_server *server = NFS_SERVER(inode);
> +	struct nfs4_state *state = ctx ? ctx->state : NULL;
>          struct nfs_setattrargs  arg = {
>                  .fh             = NFS_FH(inode),
>                  .iap            = sattr,
> @@ -2978,7 +2979,7 @@ static int nfs4_do_setattr(struct inode *inode, 
> struct rpc_cred *cred,
>  		arg.bitmask = nfs4_bitmask(server, olabel);
>
>  	do {
> -		err = _nfs4_do_setattr(inode, &arg, &res, cred, state);
> +		err = _nfs4_do_setattr(inode, &arg, &res, cred, ctx);
>  		switch (err) {
>  		case -NFS4ERR_OPENMODE:
>  			if (!(sattr->ia_valid & ATTR_SIZE)) {
> @@ -3673,7 +3674,7 @@ nfs4_proc_setattr(struct dentry *dentry, struct 
> nfs_fattr *fattr,
>  {
>  	struct inode *inode = d_inode(dentry);
>  	struct rpc_cred *cred = NULL;
> -	struct nfs4_state *state = NULL;
> +	struct nfs_open_context *ctx = NULL;
>  	struct nfs4_label *label = NULL;
>  	int status;
>
> @@ -3694,20 +3695,17 @@ nfs4_proc_setattr(struct dentry *dentry, 
> struct nfs_fattr *fattr,
>
>  	/* Search for an existing open(O_WRITE) file */
>  	if (sattr->ia_valid & ATTR_FILE) {
> -		struct nfs_open_context *ctx;
>
>  		ctx = nfs_file_open_context(sattr->ia_file);
> -		if (ctx) {
> +		if (ctx)
>  			cred = ctx->cred;
> -			state = ctx->state;
> -		}
>  	}


Does this need a get_nfs_open_context() there to make sure the open 
context
doesn't drop away?  I'm getting this on generic/089:

[  651.855291] run fstests generic/089 at 2016-11-01 11:15:57
[  652.645828] NFS: nfs4_reclaim_open_state: Lock reclaim failed!
[  653.166259] NFSD: client ::1 testing state ID with incorrect client 
ID
[  653.167218] BUG: unable to handle kernel NULL pointer dereference at 
0000000000000018
[  653.168142] IP: [<ffffffffa02dbca4>] nfs41_open_expired+0xb4/0x3c0 
[nfsv4]
[  653.168938] PGD 13470c067
[  653.169236] PUD 132ac1067
[  653.169557] PMD 0
[  653.169628]
[  653.169819] Oops: 0000 [#1] SMP
[  653.170181] Modules linked in: nfsv4 dns_resolver nfs ip6t_rpfilter 
ip6t_REJECT nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat 
ebtable_broute bridge stp llc ip6table_nat nf_conntrack_ipv6 
nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security 
ip6table_raw iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 
nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw 
ebtable_filter ebtables ip6table_filter ip6_tables nfsd auth_rpcgss 
nfs_acl lockd grace sunrpc virtio_net virtio_console virtio_blk 
virtio_balloon ppdev crct10dif_pclmul crc32_pclmul crc32c_intel 
ghash_clmulni_intel serio_raw virtio_pci parport_pc virtio_ring parport 
virtio ata_generic acpi_cpufreq pata_acpi tpm_tis i2c_piix4 tpm_tis_core 
tpm
[  653.178176] CPU: 0 PID: 7508 Comm: ::1-manager Not tainted 
4.9.0-rc1-00008-g9b7fe7e #17
[  653.179068] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS 1.8.1-20150318_183358- 04/01/2014
[  653.180125] task: ffff880139598000 task.stack: ffffc90008b18000
[  653.180777] RIP: 0010:[<ffffffffa02dbca4>]  [<ffffffffa02dbca4>] 
nfs41_open_expired+0xb4/0x3c0 [nfsv4]
[  653.181829] RSP: 0018:ffffc90008b1bd98  EFLAGS: 00010203
[  653.182420] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 
0000000000000000
[  653.183212] RDX: 0000000000000035 RSI: ffff88013fc1c9c0 RDI: 
ffff880138f2bc00
[  653.183998] RBP: ffffc90008b1bde8 R08: 000000000001c9c0 R09: 
ffff8801319a0300
[  653.184788] R10: ffff8801319a0338 R11: 0000000000000001 R12: 
00000000ffffd8d7
[  653.185577] R13: ffff88013aaeb6c0 R14: ffff88013aaeb6e0 R15: 
ffff88013946f850
[  653.186458] FS:  0000000000000000(0000) GS:ffff88013fc00000(0000) 
knlGS:0000000000000000
[  653.187329] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  653.187950] CR2: 0000000000000018 CR3: 000000013308c000 CR4: 
00000000000406f0
[  653.188723] Stack:
[  653.188951]  ffff8801319a7800 ffff8801315b9800 ffffc90008b1bda8 
ffffc90008b1bda8
[  653.189807]  000000002f556716 ffff88013aaeb6c0 ffffffffa030b460 
ffff88013946f850
[  653.190666]  ffffffffa030b460 ffff88013946f850 ffffc90008b1be80 
ffffffffa02f051f
[  653.191524] Call Trace:
[  653.191808]  [<ffffffffa02f051f>] nfs4_do_reclaim+0x1af/0x660 [nfsv4]
[  653.192522]  [<ffffffffa02f0ee0>] nfs4_run_state_manager+0x510/0x7d0 
[nfsv4]
[  653.193297]  [<ffffffffa02f09d0>] ? nfs4_do_reclaim+0x660/0x660 
[nfsv4]
[  653.194016]  [<ffffffffa02f09d0>] ? nfs4_do_reclaim+0x660/0x660 
[nfsv4]
[  653.194739]  [<ffffffff810c95d9>] kthread+0xd9/0xf0
[  653.195274]  [<ffffffff810c9500>] ? kthread_park+0x60/0x60
[  653.195875]  [<ffffffff818264d5>] ret_from_fork+0x25/0x30
[  653.196466] Code: 24 49 8b 45 40 a8 01 0f 84 f5 00 00 00 49 8b 5d 20 
4d 8d 75 20 49 39 de 75 11 e9 e3 00 00 00 48 8b 1b 4c 39 f3 0f 84 c4 00 
00 00 <48> 8b 43 18 a8 01 74 ec 48 8b 43 10 48 8b 3c 24 48 8d b3 08 01
[  653.199416] RIP  [<ffffffffa02dbca4>] nfs41_open_expired+0xb4/0x3c0 
[nfsv4]
[  653.200196]  RSP <ffffc90008b1bd98>
[  653.200578] CR2: 0000000000000018
[  653.201291] ---[ end trace ad4f1849f777932d ]---
[  653.201792] Kernel panic - not syncing: Fatal exception
[  653.202492] Kernel Offset: disabled
[  653.202876] ---[ end Kernel panic - not syncing: Fatal exception


Something else is also wrong there.. wrapping that with
get_nfs_open_context() makes the crash go away, but there are still 
several
"NFS: nfs4_reclaim_open_state: Lock reclaim failed!" in the log.   Why 
would
we be doing reclaim at all?  I'll look at a network capture next.

Ben

>
>  	label = nfs4_label_alloc(NFS_SERVER(inode), GFP_KERNEL);
>  	if (IS_ERR(label))
>  		return PTR_ERR(label);
>
> -	status = nfs4_do_setattr(inode, cred, fattr, sattr, state, NULL, 
> label);
> +	status = nfs4_do_setattr(inode, cred, fattr, sattr, ctx, NULL, 
> label);
>  	if (status == 0) {
>  		nfs_setattr_update_inode(inode, sattr, fattr);
>  		nfs_setsecurity(inode, fattr, label);
>

Re: [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.

[NeilBrown]

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

On Thu, Nov 03 2016, Benjamin Coddington wrote:

> On 13 Oct 2016, at 0:26, NeilBrown wrote:

>> @@ -3694,20 +3695,17 @@ nfs4_proc_setattr(struct dentry *dentry, 
>> struct nfs_fattr *fattr,
>>
>>  	/* Search for an existing open(O_WRITE) file */
>>  	if (sattr->ia_valid & ATTR_FILE) {
>> -		struct nfs_open_context *ctx;
>>
>>  		ctx = nfs_file_open_context(sattr->ia_file);
>> -		if (ctx) {
>> +		if (ctx)
>>  			cred = ctx->cred;
>> -			state = ctx->state;
>> -		}
>>  	}
>
>
> Does this need a get_nfs_open_context() there to make sure the open 
> context
> doesn't drop away?

I can't see why you would.  The ia_file must hold a reference to the
ctx, and this code doesn't keep any reference to the ctx after
nfs4_proc_setattr completes - does it?



>     I'm getting this on generic/089:
>
> [  651.855291] run fstests generic/089 at 2016-11-01 11:15:57
> [  652.645828] NFS: nfs4_reclaim_open_state: Lock reclaim failed!
> [  653.166259] NFSD: client ::1 testing state ID with incorrect client 
> ID
> [  653.167218] BUG: unable to handle kernel NULL pointer dereference at 
> 0000000000000018

I think this BUG is happening in nfs41_check_expired_locks.
This:
	list_for_each_entry(lsp, &state->lock_states, ls_locks) {
walks off the end of a list, finding a NULL on a list which should never
have a NULL pointer.  That does suggest a use-after-free of an
nfs4_lock_state, or possibly of an nfs4_state.

I can't see it in the code yet though.

>
> Something else is also wrong there.. wrapping that with
> get_nfs_open_context() makes the crash go away, but there are still 
> several
> "NFS: nfs4_reclaim_open_state: Lock reclaim failed!" in the log.   Why 
> would
> we be doing reclaim at all?  I'll look at a network capture next.

The
> [  653.166259] NFSD: client ::1 testing state ID with incorrect client ID

errors suggests that the client is sending a stateid that doesn't match
the client id, so the server reports and error and the client enters
state recovery.
Maybe one thread is dropping a flock lock while another thread is using
it for some IO and they race?  I think there are refcounts in place to
protect that but something might be missing.

I look forward to seeing the network capture.

Thanks,
NeilBrown

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

Re: [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.

[Benjamin Coddington]

On 2 Nov 2016, at 19:34, NeilBrown wrote:

> On Thu, Nov 03 2016, Benjamin Coddington wrote:
>
>> On 13 Oct 2016, at 0:26, NeilBrown wrote:
>
>>> @@ -3694,20 +3695,17 @@ nfs4_proc_setattr(struct dentry *dentry,
>>> struct nfs_fattr *fattr,
>>>
>>>  	/* Search for an existing open(O_WRITE) file */
>>>  	if (sattr->ia_valid & ATTR_FILE) {
>>> -		struct nfs_open_context *ctx;
>>>
>>>  		ctx = nfs_file_open_context(sattr->ia_file);
>>> -		if (ctx) {
>>> +		if (ctx)
>>>  			cred = ctx->cred;
>>> -			state = ctx->state;
>>> -		}
>>>  	}
>>
>>
>> Does this need a get_nfs_open_context() there to make sure the open
>> context
>> doesn't drop away?
>
> I can't see why you would.  The ia_file must hold a reference to the
> ctx, and this code doesn't keep any reference to the ctx after
> nfs4_proc_setattr completes - does it?

I had a thought that the file pointer could be shared, and just checking to
see if the file's private data was set didn't mean that this task set it.
I'm probably wrong about that, and I'm definitely wrong about the fix, since
it will crash taking a reference.  It just took much longer this time.

>>     I'm getting this on generic/089:
>>
>> [  651.855291] run fstests generic/089 at 2016-11-01 11:15:57
>> [  652.645828] NFS: nfs4_reclaim_open_state: Lock reclaim failed!
>> [  653.166259] NFSD: client ::1 testing state ID with incorrect client
>> ID
>> [  653.167218] BUG: unable to handle kernel NULL pointer dereference at
>> 0000000000000018
>
> I think this BUG is happening in nfs41_check_expired_locks.
> This:
> 	list_for_each_entry(lsp, &state->lock_states, ls_locks) {
> walks off the end of a list, finding a NULL on a list which should never
> have a NULL pointer.  That does suggest a use-after-free of an
> nfs4_lock_state, or possibly of an nfs4_state.
>
> I can't see it in the code yet though.

Yes, walking off the list, and I had some idea that we were getting
duplicate fl_owners from a bad lock_context, but today that doesn't make
any sense to me.  I can't see how this can happen either.

>>
>> Something else is also wrong there.. wrapping that with
>> get_nfs_open_context() makes the crash go away, but there are still
>> several
>> "NFS: nfs4_reclaim_open_state: Lock reclaim failed!" in the log.   Why
>> would
>> we be doing reclaim at all?  I'll look at a network capture next.
>
> The
>> [  653.166259] NFSD: client ::1 testing state ID with incorrect client ID
>
> errors suggests that the client is sending a stateid that doesn't match
> the client id, so the server reports and error and the client enters
> state recovery.
> Maybe one thread is dropping a flock lock while another thread is using
> it for some IO and they race?  I think there are refcounts in place to
> protect that but something might be missing.
>
> I look forward to seeing the network capture.

The network capture isn't very enlightening.. or I haven't been able to find
where the problem happens in the 800Mb of capture.  I probably need to make
the box panic on oops and capture externally so the problem is right at the
end of the capture.  I'll keep plugging away at this as I can.

Ben

Re: [PATCH 3/6] NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.

[Benjamin Coddington]

On 3 Nov 2016, at 12:38, Benjamin Coddington wrote:
> The network capture isn't very enlightening.. or I haven't been able 
> to find
> where the problem happens in the 800Mb of capture.  I probably need to 
> make
> the box panic on oops and capture externally so the problem is right 
> at the
> end of the capture.  I'll keep plugging away at this as I can.

I just got this on v4.9-rc1 without these patches, so false alarm here.  
Sorry
for the noise.  I'll see if I can bisect.

Ben

Re: [PATCH 0/6] NFSv4: Fix stateid used when flock locks in use. - V2

[NeilBrown]

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


Hi all,
 what is the status here.  I don't see these in linux-next, nor do I see
 any issues raise with them.
 Is there any chance they can get into 4.10??

Thanks,
NeilBrown


On Wed, Oct 19 2016, NeilBrown wrote:

> [ Unknown signature status ]
> On Fri, Oct 14 2016, Jeff Layton wrote:
>
>>
>> This all looks good to me aside from some minor documentation nits.
>> Kudos too on doing this with a net removal of code. :)
>>
>> Reviewed-by: Jeff Layton <jlayton@redhat.com>
>
> Thanks.
>
> The full series, with Reviewed-by added, is available as below if
> a "git pull" is easier that plucking them out of emails
>
> NeilBrown
>
>
> The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:
>
>   Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)
>
> are available in the git repository at:
>
>   git://neil.brown.name/linux tags/nfs-flock-fix
>
> for you to fetch changes up to 077829bfd0e32131b49d87476b69a189e8e13ae8:
>
>   NFS: discard nfs_lockowner structure. (2016-10-19 08:44:25 +1100)
>
> ----------------------------------------------------------------
> Series to fix NFSv4 stateid used for flock locks
>
> ----------------------------------------------------------------
> NeilBrown (6):
>       NFS: remove l_pid field from nfs_lockowner
>       NFSv4: add flock_owner to open context
>       NFSv4: change nfs4_do_setattr to take an open_context instead of a nfs4_state.
>       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      | 44 +++++++++++++++++++-------------------------
>  fs/nfs/nfs4state.c     | 49 ++++++++++++++++++++++++++++++++-----------------
>  fs/nfs/pagelist.c      |  3 +--
>  fs/nfs/write.c         |  3 +--
>  include/linux/nfs_fs.h | 10 +++-------
>  9 files changed, 68 insertions(+), 65 deletions(-)

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

[PATCH] NFSv4: ensure __nfs4_find_lock_state returns consistent result.

[NeilBrown]

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


If a file has both flock locks and OFD locks, then it is possible that
two different nfs4 lock states could apply to file accesses from a
single process.

It is not possible to know, efficiently, which one is "correct".
Presumably the state which represents a lock that covers the region
undergoing IO would be the "correct" one to use, but finding that has
a non-trivial cost and would provide miniscule value.

Currently we just return whichever is first in the list, which could
result in inconsistent behaviour if an application ever put it self in
this position.  As consistent behaviour is preferable (when perfectly
correct behaviour is not available), change the search to return a
consistent result in this circumstance.
Specifically: if there is both a flock and OFD lock state, always return
the flock one.

Reviewed-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---

Hi Trond,
 thanks for applying my recent lock-state management series.
 This patch was missed, probably because it was buried in a reply to a
 conversation.  Sorry about that.

 It is not a critical fix, but it does complete the series.

Thanks,
NeilBrown


 fs/nfs/nfs4state.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 95baf7d340f0..cf869802ff23 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -797,21 +797,33 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode)
 
 /*
  * Search the state->lock_states for an existing lock_owner
- * that is compatible with current->files
+ * that is compatible with either of the given owners.
+ * If the second is non-zero, then the first refers to a Posix-lock
+ * owner (current->files) and the second refers to a flock/OFD
+ * owner (struct file*).  In that case, prefer a match for the first
+ * owner.
+ * If both sorts of locks are held on the one file we cannot know
+ * which stateid was intended to be used, so a "correct" choice cannot
+ * be made.  Failing that, a "consistent" choice is preferable.  The
+ * consistent choice we make is to prefer the first owner, that of a
+ * Posix lock.
  */
 static struct nfs4_lock_state *
 __nfs4_find_lock_state(struct nfs4_state *state,
 		       fl_owner_t fl_owner, fl_owner_t fl_owner2)
 {
-	struct nfs4_lock_state *pos;
+	struct nfs4_lock_state *pos, *ret = NULL;
 	list_for_each_entry(pos, &state->lock_states, ls_locks) {
-		if (pos->ls_owner != fl_owner &&
-		    pos->ls_owner != fl_owner2)
-			continue;
-		atomic_inc(&pos->ls_count);
-		return pos;
+		if (pos->ls_owner == fl_owner) {
+			ret = pos;
+			break;
+		}
+		if (pos->ls_owner == fl_owner2)
+			ret = pos;
 	}
-	return NULL;
+	if (ret)
+		atomic_inc(&ret->ls_count);
+	return ret;
 }
 
 /*
-- 
2.11.0


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