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

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

[NeilBrown]

As mentioned in a previous email, NFSv4 currently uses the wrong
stateid when a 'flock' lock is active.
It should use the stateid returned for the LOCK request, but it
actually uses the stateid returned by the OPEN request.
If a server implements mandatory locks, this causes an error on
read/write requests.

This is a regression introduced by
 Commit: 8003d3c4aaa5 ("nfs4: treat lock owners as opaque values")

This patch set fixes the problem but is not perfect.

I added a second 'nfs_lock_context' to 'struct nfs_open_context'
to represent the flock context.
Only the lockowner.l_owner field of this nfs_lock_context is used,
so we could get away with only adding a 'fl_owner_t flock_owner'
field.  I left the full nfs_lock_context as I wasn't sure about
the io_count field...

I haven't made any changes to the ->io_count handling.  This is
because it isn't at all clear to me that the current code is correct.

I assume the io_count tracking is there to ensure that we don't
unlock a lock while there are outstanding reads that might be using
the stateid - there shouldn't be any writes because we just called
fsync.

However I see two problems:
1/ the io_count is per-process and per-'struct file'.  It is quite
  possible for a process to open the same file twice, so it has
  two 'struct file' on the same file, and thus two nfs_open_context
  and two nfs_lock_context, and two io_count.  Both of these would
  use the one per-process/per-inode stateid.
  Waiting for one io_count to reach zero doesn't help if the other
  io_count is not zero.

2/ I cannot see any locking that would prevent the stateid from
   being used again (e.g. by another thread) immediately after
   the call to nfs_iocounter_wait().

I wonder if the io_count should be attached to the nfs4_lock_state
rather than the nfs_lock_context???
Maybe I am misunderstanding the whole point of io_count, but in any
case I did not think I could make any useful changes there, so I
haven't.

I haven't considered how these changes interact with OFD locks.
They might just transparently start working once flock locks
are working again.  Of course we still have the issue that
if a process has a Posix lock on one byte-range and an OFD lock
on another byte range, then that is difficult to cope with.
I doubt the value for "fixing" this is worth the cost.

Comments very welcome.

Thanks,
NeilBrown


---

NeilBrown (3):
      NFSv4: add flock_context 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


 fs/nfs/dir.c           |    6 +++---
 fs/nfs/inode.c         |   16 ++++++++++++++--
 fs/nfs/nfs4_fs.h       |    2 +-
 fs/nfs/nfs4file.c      |    2 +-
 fs/nfs/nfs4proc.c      |   12 +++---------
 fs/nfs/nfs4state.c     |   34 +++++++++++++++++++++-------------
 include/linux/nfs_fs.h |    3 ++-
 7 files changed, 45 insertions(+), 30 deletions(-)

--
Signature

[PATCH 1/3] NFSv4: add flock_context to open context

[NeilBrown]

An open file description (struct file) can have two possible lock contexts.

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

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

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

This flock_context does not need to be reference-counted (as we will
use the refcount for the primary lock_context), and will never be part
of a list of contexts.  So initilization is minimal.

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

The flock_context 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         |   16 ++++++++++++++--
 fs/nfs/nfs4file.c      |    2 +-
 fs/nfs/nfs4proc.c      |    2 +-
 include/linux/nfs_fs.h |    3 ++-
 5 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 177fefb26c18..797dbaf1ebe4 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 bf4ec5ecc97e..40e7b88f680d 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -708,6 +708,14 @@ static void nfs_init_lock_context(struct nfs_lock_context *l_ctx)
 	atomic_set(&l_ctx->io_count, 0);
 }
 
+static void nfs_init_flock_context(struct nfs_lock_context *l_ctx, struct file *filp)
+{
+	atomic_set(&l_ctx->count, 0);
+	l_ctx->lockowner.l_owner = filp;
+	l_ctx->lockowner.l_pid = 0;
+	atomic_set(&l_ctx->io_count, 0);
+}
+
 static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context *ctx)
 {
 	struct nfs_lock_context *head = &ctx->lock_context;
@@ -799,7 +807,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();
@@ -819,7 +829,9 @@ struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f
 	ctx->flags = 0;
 	ctx->error = 0;
 	nfs_init_lock_context(&ctx->lock_context);
+	nfs_init_flock_context(&ctx->flock_context, filp);
 	ctx->lock_context.open_context = ctx;
+	ctx->flock_context.open_context = ctx;
 	INIT_LIST_HEAD(&ctx->list);
 	ctx->mdsthreshold = NULL;
 	return ctx;
@@ -942,7 +954,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 d085ad794884..13b899b9347e 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 a9dec32ba9ba..a8f8b4720c90 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3793,7 +3793,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 810124b33327..a9fb754fff34 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -71,6 +71,7 @@ struct nfs_lock_context {
 struct nfs4_state;
 struct nfs_open_context {
 	struct nfs_lock_context lock_context;
+	struct nfs_lock_context flock_context;
 	struct dentry *dentry;
 	struct rpc_cred *cred;
 	struct nfs4_state *state;
@@ -358,7 +359,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/3] NFSv4: enhance nfs4_copy_lock_stateid to use a flock stateid if there is one

[NeilBrown]

A process can have two possible lock contexts for a given open file:
a per-process Posix lock context and a per-open-file flock lock context.
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..74cc32490c7a 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_context.lockowner.l_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 2/3] 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 wont 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  |   10 ++--------
 fs/nfs/nfs4state.c |   19 +++++++++++--------
 3 files changed, 14 insertions(+), 17 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 a8f8b4720c90..05ae1cecf9f7 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2764,13 +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,
-			.l_pid = current->tgid,
-		};
 		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
@@ -4365,9 +4361,7 @@ int nfs4_set_rw_stateid(nfs4_stateid *stateid,
 {
 	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;