All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/6] Skipped unlocks
@ 2017-04-06 11:23 Benjamin Coddington
  2017-04-06 11:23 ` [PATCH 1/6] NFS4: remove a redundant lock range check Benjamin Coddington
                   ` (5 more replies)
  0 siblings, 6 replies; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-06 11:23 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, Jeff Layton, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

Well over a year ago I made other attempts to fix the problem of NFS failing
to send an unlock when signalled.  Those attempts were terrible.

Here's yet another version, this time using the approach suggested by Jeff
Layton to utilize an RPC waitqueue to defer unlock tasks until IO has
completed.  In order to do this for NLM without violating all the layers,
a new nlmclnt_operations struct is defined and passed in to allow the NLM
client to call back out to NFS at various stages.

Comments and review are welcomed.

since v1:
        - add Christoph's reviewed-by on 1/4 and 2/4 and fixup switch
          indentation on 2/4
since v2:
        - don't sleep in rpciod to wait for I/O completion, just send the unlock
          immediately for both v3 and v4.
since v3:
		- don't just skip the wait for FL_CLOSE.  Instead, build new
		  machinery into NLM to allow waiting in rpc_call_prepare().
since v4:
		- move the uoc_rpcwaitq outside #if IS_ENABLED(CONFIG_NFS_V4), since
		  we need it for nfsv3.  Add some explanation for the fuse change in 3/6.

Benjamin Coddington (6):
  NFS4: remove a redundant lock range check
  NFS: Move the flock open mode check into nfs_flock()
  locks: Set FL_CLOSE when removing flock locks on close()
  NFS: Add an iocounter wait function for async RPC tasks
  lockd: Introduce nlmclnt_operations
  NFS: Always wait for I/O completion before unlock

 fs/fuse/file.c              |  2 +-
 fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
 fs/locks.c                  |  2 +-
 fs/nfs/client.c             |  1 +
 fs/nfs/file.c               | 28 +++++++++++++++++++++-------
 fs/nfs/nfs3proc.c           | 38 +++++++++++++++++++++++++++++++++++++-
 fs/nfs/nfs4proc.c           | 27 +++++++--------------------
 fs/nfs/pagelist.c           | 26 +++++++++++++++++++++++++-
 fs/nfs/proc.c               |  2 +-
 include/linux/fs.h          |  2 ++
 include/linux/lockd/bind.h  | 11 +++++++++--
 include/linux/lockd/lockd.h |  2 ++
 include/linux/nfs_fs.h      |  1 +
 include/linux/nfs_fs_sb.h   |  1 +
 include/linux/nfs_page.h    |  1 +
 15 files changed, 135 insertions(+), 35 deletions(-)

-- 
2.9.3

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/6] NFS4: remove a redundant lock range check
  2017-04-06 11:23 [PATCH v5 0/6] Skipped unlocks Benjamin Coddington
@ 2017-04-06 11:23 ` Benjamin Coddington
  2017-04-06 11:23 ` [PATCH 2/6] NFS: Move the flock open mode check into nfs_flock() Benjamin Coddington
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-06 11:23 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, Jeff Layton, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

flock64_to_posix_lock() is already doing this check

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
Reviewed-by: Jeff Layton <jeff.layton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/nfs4proc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 0a0eaecf9676..9388899e4050 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6570,9 +6570,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 	ctx = nfs_file_open_context(filp);
 	state = ctx->state;
 
-	if (request->fl_start < 0 || request->fl_end < 0)
-		return -EINVAL;
-
 	if (IS_GETLK(cmd)) {
 		if (state != NULL)
 			return nfs4_proc_getlk(state, F_GETLK, request);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/6] NFS: Move the flock open mode check into nfs_flock()
  2017-04-06 11:23 [PATCH v5 0/6] Skipped unlocks Benjamin Coddington
  2017-04-06 11:23 ` [PATCH 1/6] NFS4: remove a redundant lock range check Benjamin Coddington
@ 2017-04-06 11:23 ` Benjamin Coddington
  2017-04-06 11:23 ` [PATCH 3/6] locks: Set FL_CLOSE when removing flock locks on close() Benjamin Coddington
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-06 11:23 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, Jeff Layton, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

We only need to check lock exclusive/shared types against open mode when
flock() is used on NFS, so move it into the flock-specific path instead of
checking it for all locks.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/file.c     | 18 ++++++++++++++++--
 fs/nfs/nfs4proc.c | 14 --------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 26dbe8b0c10d..a490f45df4db 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -820,9 +820,23 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
 	if (NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK)
 		is_local = 1;
 
-	/* We're simulating flock() locks using posix locks on the server */
-	if (fl->fl_type == F_UNLCK)
+	/*
+	 * VFS doesn't require the open mode to match a flock() lock's type.
+	 * NFS, however, may simulate flock() locking with posix locking which
+	 * requires the open mode to match the lock type.
+	 */
+	switch (fl->fl_type) {
+	case F_UNLCK:
 		return do_unlk(filp, cmd, fl, is_local);
+	case F_RDLCK:
+		if (!(filp->f_mode & FMODE_READ))
+			return -EBADF;
+		break;
+	case F_WRLCK:
+		if (!(filp->f_mode & FMODE_WRITE))
+			return -EBADF;
+	}
+
 	return do_setlk(filp, cmd, fl, is_local);
 }
 EXPORT_SYMBOL_GPL(nfs_flock);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9388899e4050..91f88bfbbe79 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6592,20 +6592,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 	    !test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
 		return -ENOLCK;
 
-	/*
-	 * Don't rely on the VFS having checked the file open mode,
-	 * since it won't do this for flock() locks.
-	 */
-	switch (request->fl_type) {
-	case F_RDLCK:
-		if (!(filp->f_mode & FMODE_READ))
-			return -EBADF;
-		break;
-	case F_WRLCK:
-		if (!(filp->f_mode & FMODE_WRITE))
-			return -EBADF;
-	}
-
 	status = nfs4_set_lock_state(state, request);
 	if (status != 0)
 		return status;
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/6] locks: Set FL_CLOSE when removing flock locks on close()
  2017-04-06 11:23 [PATCH v5 0/6] Skipped unlocks Benjamin Coddington
  2017-04-06 11:23 ` [PATCH 1/6] NFS4: remove a redundant lock range check Benjamin Coddington
  2017-04-06 11:23 ` [PATCH 2/6] NFS: Move the flock open mode check into nfs_flock() Benjamin Coddington
@ 2017-04-06 11:23 ` Benjamin Coddington
  2017-04-06 18:17     ` Jeff Layton
  2017-04-08  5:34   ` kbuild test robot
  2017-04-06 11:23 ` [PATCH 4/6] NFS: Add an iocounter wait function for async RPC tasks Benjamin Coddington
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-06 11:23 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, Jeff Layton, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks.
NFS will check for this flag to ensure an unlock is sent in a following
patch.

Fuse handles flock and posix locks differently for FL_CLOSE, and so
requires a fixup to retain the existing behavior for flock.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/fuse/file.c     | 2 +-
 fs/locks.c         | 2 +-
 include/linux/fs.h | 2 ++
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 2401c5dabb2a..79e1a3a048a8 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2172,7 +2172,7 @@ static int fuse_setlk(struct file *file, struct file_lock *fl, int flock)
 	}
 
 	/* Unlock on close is handled by the flush method */
-	if (fl->fl_flags & FL_CLOSE)
+	if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
 		return 0;
 
 	fuse_lk_fill(&args, file, fl, opcode, pid, flock, &inarg);
diff --git a/fs/locks.c b/fs/locks.c
index 26811321d39b..af2031a1fcff 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
 		.fl_owner = filp,
 		.fl_pid = current->tgid,
 		.fl_file = filp,
-		.fl_flags = FL_FLOCK,
+		.fl_flags = FL_FLOCK | FL_CLOSE,
 		.fl_type = F_UNLCK,
 		.fl_end = OFFSET_MAX,
 	};
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2ba074328894..e964ae0a9c29 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -903,6 +903,8 @@ static inline struct file *get_file(struct file *f)
 #define FL_OFDLCK	1024	/* lock is "owned" by struct file */
 #define FL_LAYOUT	2048	/* outstanding pNFS layout */
 
+#define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
+
 /*
  * Special return value from posix_lock_file() and vfs_lock_file() for
  * asynchronous locking.
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/6] NFS: Add an iocounter wait function for async RPC tasks
  2017-04-06 11:23 [PATCH v5 0/6] Skipped unlocks Benjamin Coddington
                   ` (2 preceding siblings ...)
  2017-04-06 11:23 ` [PATCH 3/6] locks: Set FL_CLOSE when removing flock locks on close() Benjamin Coddington
@ 2017-04-06 11:23 ` Benjamin Coddington
  2017-04-07 10:44     ` Jeff Layton
  2017-04-06 11:23 ` [PATCH 5/6] lockd: Introduce nlmclnt_operations Benjamin Coddington
  2017-04-06 11:23 ` [PATCH 6/6] NFS: Always wait for I/O completion before unlock Benjamin Coddington
  5 siblings, 1 reply; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-06 11:23 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, Jeff Layton, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

By sleeping on a new NFS Unlock-On-Close waitqueue, rpc tasks may wait for
a lock context's iocounter to reach zero.  The rpc waitqueue is only woken
when the open_context has the NFS_CONTEXT_UNLOCK flag set in order to
mitigate spurious wake-ups for any iocounter reaching zero.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/client.c           |  1 +
 fs/nfs/pagelist.c         | 26 +++++++++++++++++++++++++-
 include/linux/nfs_fs.h    |  1 +
 include/linux/nfs_fs_sb.h |  1 +
 include/linux/nfs_page.h  |  1 +
 5 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 91a8d610ba0f..c335c6dce285 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -218,6 +218,7 @@ static void nfs_cb_idr_remove_locked(struct nfs_client *clp)
 static void pnfs_init_server(struct nfs_server *server)
 {
 	rpc_init_wait_queue(&server->roc_rpcwaitq, "pNFS ROC");
+	rpc_init_wait_queue(&server->uoc_rpcwaitq, "NFS UOC");
 }
 
 #else
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 6e629b856a00..d163819e4416 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -115,6 +115,27 @@ nfs_iocounter_wait(struct nfs_lock_context *l_ctx)
 			TASK_KILLABLE);
 }
 
+bool
+nfs_async_iocounter_wait(struct rpc_task *task, void *data)
+{
+	struct nfs_lock_context *l_ctx = data;
+	struct inode *inode = d_inode(l_ctx->open_context->dentry);
+	bool ret = false;
+
+	if (atomic_read(&l_ctx->io_count) > 0) {
+		rpc_sleep_on(&NFS_SERVER(inode)->uoc_rpcwaitq, task, NULL);
+		ret = true;
+	}
+
+	if (atomic_read(&l_ctx->io_count) == 0) {
+		rpc_wake_up_queued_task(&NFS_SERVER(inode)->uoc_rpcwaitq, task);
+		ret = false;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nfs_async_iocounter_wait);
+
 /*
  * nfs_page_group_lock - lock the head of the page group
  * @req - request in group that is to be locked
@@ -398,8 +419,11 @@ static void nfs_clear_request(struct nfs_page *req)
 		req->wb_page = NULL;
 	}
 	if (l_ctx != NULL) {
-		if (atomic_dec_and_test(&l_ctx->io_count))
+		if (atomic_dec_and_test(&l_ctx->io_count)) {
 			wake_up_atomic_t(&l_ctx->io_count);
+			if (test_bit(NFS_CONTEXT_UNLOCK, &ctx->flags))
+				rpc_wake_up(&NFS_SERVER(d_inode(ctx->dentry))->uoc_rpcwaitq);
+		}
 		nfs_put_lock_context(l_ctx);
 		req->wb_lock_context = NULL;
 	}
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index f1da8c8dd473..3ce3e42beead 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -76,6 +76,7 @@ struct nfs_open_context {
 #define NFS_CONTEXT_ERROR_WRITE		(0)
 #define NFS_CONTEXT_RESEND_WRITES	(1)
 #define NFS_CONTEXT_BAD			(2)
+#define NFS_CONTEXT_UNLOCK	(3)
 	int error;
 
 	struct list_head list;
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index b34097c67848..2a70f34dffe8 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -222,6 +222,7 @@ struct nfs_server {
 	u32			mountd_version;
 	unsigned short		mountd_port;
 	unsigned short		mountd_protocol;
+	struct rpc_wait_queue	uoc_rpcwaitq;
 };
 
 /* Server capabilities */
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index 957049f72290..d39ebcba52c8 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -141,6 +141,7 @@ extern int nfs_page_group_lock(struct nfs_page *, bool);
 extern void nfs_page_group_lock_wait(struct nfs_page *);
 extern void nfs_page_group_unlock(struct nfs_page *);
 extern bool nfs_page_group_sync_on_bit(struct nfs_page *, unsigned int);
+extern bool nfs_async_iocounter_wait(struct rpc_task *, void *);
 
 /*
  * Lock the page of an asynchronous request
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 5/6] lockd: Introduce nlmclnt_operations
  2017-04-06 11:23 [PATCH v5 0/6] Skipped unlocks Benjamin Coddington
                   ` (3 preceding siblings ...)
  2017-04-06 11:23 ` [PATCH 4/6] NFS: Add an iocounter wait function for async RPC tasks Benjamin Coddington
@ 2017-04-06 11:23 ` Benjamin Coddington
  2017-04-07 12:10     ` Jeff Layton
  2017-04-06 11:23 ` [PATCH 6/6] NFS: Always wait for I/O completion before unlock Benjamin Coddington
  5 siblings, 1 reply; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-06 11:23 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, Jeff Layton, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

NFS would enjoy the ability to modify the behavior of the NLM client's
unlock RPC task in order to delay the transmission of the unlock until IO
that was submitted under that lock has completed.  This ability can ensure
that the NLM client will always complete the transmission of an unlock even
if the waiting caller has been interrupted with fatal signal.

A struct nlmclnt_operations and callback data pointer can be passed to
nlmclnt_proc(). The struct nlmclnt_operations defines three callback
operations that will be used in a following patch:

nlmclnt_alloc_call - used to call back after a successful allocation of
	a struct nlm_rqst in nlmclnt_proc().

nlmclnt_unlock_prepare - used to call back during NLM unlock's
	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
	until this callback returns false.

nlmclnt_release_call - used to call back when the NLM client's struct
	nlm_rqst is freed.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
 fs/nfs/nfs3proc.c           |  2 +-
 fs/nfs/proc.c               |  2 +-
 include/linux/lockd/bind.h  | 11 +++++++++--
 include/linux/lockd/lockd.h |  2 ++
 5 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 112952037933..d2744f4960ed 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -150,9 +150,12 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
  * @host: address of a valid nlm_host context representing the NLM server
  * @cmd: fcntl-style file lock operation to perform
  * @fl: address of arguments for the lock operation
+ * @nlmclnt_ops: operations to call back out of nlm
+ * @data: address of data to be sent along with callback
  *
  */
-int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
+int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
+		const struct nlmclnt_operations *nlmclnt_ops, void *data)
 {
 	struct nlm_rqst		*call;
 	int			status;
@@ -161,6 +164,9 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
 	if (call == NULL)
 		return -ENOMEM;
 
+	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
+		nlmclnt_ops->nlmclnt_alloc_call(data);
+
 	nlmclnt_locks_init_private(fl, host);
 	if (!fl->fl_u.nfs_fl.owner) {
 		/* lockowner allocation has failed */
@@ -169,6 +175,8 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
 	}
 	/* Set up the argument struct */
 	nlmclnt_setlockargs(call, fl);
+	call->nlmclnt_ops = nlmclnt_ops;
+	call->callback_data = data;
 
 	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
 		if (fl->fl_type != F_UNLCK) {
@@ -216,6 +224,8 @@ void nlmclnt_release_call(struct nlm_rqst *call)
 {
 	if (!atomic_dec_and_test(&call->a_count))
 		return;
+	if (call->nlmclnt_ops && call->nlmclnt_ops->nlmclnt_release_call)
+		call->nlmclnt_ops->nlmclnt_release_call(call->callback_data);
 	nlmclnt_release_host(call->a_host);
 	nlmclnt_release_lockargs(call);
 	kfree(call);
@@ -687,6 +697,19 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
 	return status;
 }
 
+static void nlmclnt_unlock_prepare(struct rpc_task *task, void *data)
+{
+	struct nlm_rqst	*req = data;
+	const struct nlmclnt_operations *nlmclnt_ops = req->nlmclnt_ops;
+	bool defer_call = false;
+
+	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_unlock_prepare)
+		defer_call = nlmclnt_ops->nlmclnt_unlock_prepare(task, req->callback_data);
+
+	if (!defer_call)
+		rpc_call_start(task);
+}
+
 static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
 {
 	struct nlm_rqst	*req = data;
@@ -720,6 +743,7 @@ static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
 }
 
 static const struct rpc_call_ops nlmclnt_unlock_ops = {
+	.rpc_call_prepare = nlmclnt_unlock_prepare,
 	.rpc_call_done = nlmclnt_unlock_callback,
 	.rpc_release = nlmclnt_rpc_release,
 };
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index dc925b531f32..086623ab07b1 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -870,7 +870,7 @@ nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = file_inode(filp);
 
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
 }
 
 static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index b7bca8303989..fb8cbdf0a980 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -638,7 +638,7 @@ nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = file_inode(filp);
 
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
 }
 
 /* Helper functions for NFS lock bounds checking */
diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
index 140edab64446..00c3c2a62bb5 100644
--- a/include/linux/lockd/bind.h
+++ b/include/linux/lockd/bind.h
@@ -18,6 +18,7 @@
 
 /* Dummy declarations */
 struct svc_rqst;
+struct rpc_task;
 
 /*
  * This is the set of functions for lockd->nfsd communication
@@ -52,8 +53,14 @@ struct nlmclnt_initdata {
 extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
 extern void	nlmclnt_done(struct nlm_host *host);
 
-extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
-					struct file_lock *fl);
+struct nlmclnt_operations {
+	void (*nlmclnt_alloc_call)(void *);
+	bool (*nlmclnt_unlock_prepare)(struct rpc_task*, void *);
+	void (*nlmclnt_release_call)(void *);
+};
+
+extern int	nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
+	const struct nlmclnt_operations *nlmclnt_ops, void *data);
 extern int	lockd_up(struct net *net);
 extern void	lockd_down(struct net *net);
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index c15373894a42..0c6f777dde53 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -142,6 +142,8 @@ struct nlm_rqst {
 	struct nlm_block *	a_block;
 	unsigned int		a_retries;	/* Retry count */
 	u8			a_owner[NLMCLNT_OHSIZE];
+	const struct nlmclnt_operations * nlmclnt_ops;
+	void * callback_data;
 };
 
 /*
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 6/6] NFS: Always wait for I/O completion before unlock
  2017-04-06 11:23 [PATCH v5 0/6] Skipped unlocks Benjamin Coddington
                   ` (4 preceding siblings ...)
  2017-04-06 11:23 ` [PATCH 5/6] lockd: Introduce nlmclnt_operations Benjamin Coddington
@ 2017-04-06 11:23 ` Benjamin Coddington
  2017-04-07 12:22     ` Jeff Layton
  5 siblings, 1 reply; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-06 11:23 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, Jeff Layton, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

NFS attempts to wait for read and write completion before unlocking in
order to ensure that the data returned was protected by the lock.  When
this waiting is interrupted by a signal, the unlock may be skipped, and
messages similar to the following are seen in the kernel ring buffer:

[20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
[20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 fl_pid=20183
[20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 fl_pid=20185

For NFSv3, the missing unlock will cause the server to refuse conflicting
locks indefinitely.  For NFSv4, the leftover lock will be removed by the
server after the lease timeout.

This patch fixes this issue by skipping the usual wait in
nfs_iocounter_wait if the FL_CLOSE flag is set when signaled.  Instead, the
wait happens in the unlock RPC task on the NFS UOC rpc_waitqueue.

For NFSv3, use lockd's new nlmclnt_operations along with
nfs_async_iocounter_wait to defer NLM's unlock task until the lock
context's iocounter reaches zero.

For NFSv4, call nfs_async_iocounter_wait() directly from unlock's
current rpc_call_prepare.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/file.c     | 10 +++++-----
 fs/nfs/nfs3proc.c | 38 +++++++++++++++++++++++++++++++++++++-
 fs/nfs/nfs4proc.c | 10 +++++++---
 3 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index a490f45df4db..df695f52bb9d 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	if (!IS_ERR(l_ctx)) {
 		status = nfs_iocounter_wait(l_ctx);
 		nfs_put_lock_context(l_ctx);
-		if (status < 0)
+		/*  NOTE: special case
+		 * 	If we're signalled while cleaning up locks on process exit, we
+		 * 	still need to complete the unlock.
+		 */
+		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
 			return status;
 	}
 
-	/* NOTE: special case
-	 * 	If we're signalled while cleaning up locks on process exit, we
-	 * 	still need to complete the unlock.
-	 */
 	/*
 	 * Use local locking if mounted with "-onolock" or with appropriate
 	 * "-olocal_lock="
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 086623ab07b1..0e21306bfed9 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -865,12 +865,48 @@ static void nfs3_proc_commit_setup(struct nfs_commit_data *data, struct rpc_mess
 	msg->rpc_proc = &nfs3_procedures[NFS3PROC_COMMIT];
 }
 
+void nfs3_nlm_alloc_call(void *data)
+{
+	struct nfs_lock_context *l_ctx = data;
+	nfs_get_lock_context(l_ctx->open_context);
+}
+
+void nfs3_nlm_release_call(void *data)
+{
+	struct nfs_lock_context *l_ctx = data;
+	nfs_put_lock_context(l_ctx);
+}
+
+const struct nlmclnt_operations nlmclnt_fl_close_lock_ops = {
+	.nlmclnt_alloc_call = nfs3_nlm_alloc_call,
+	.nlmclnt_unlock_prepare = nfs_async_iocounter_wait,
+	.nlmclnt_release_call = nfs3_nlm_release_call,
+};
+
 static int
 nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = file_inode(filp);
+	struct nfs_lock_context *l_ctx = NULL;
+	const struct nlmclnt_operations *nlmclnt_ops = NULL;
+	int status;
 
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
+	if (fl->fl_flags & FL_CLOSE) {
+		l_ctx = nfs_get_lock_context(nfs_file_open_context(filp));
+		if (IS_ERR(l_ctx)) {
+			l_ctx = NULL;
+		} else {
+			set_bit(NFS_CONTEXT_UNLOCK, &l_ctx->open_context->flags);
+			nlmclnt_ops = &nlmclnt_fl_close_lock_ops;
+		}
+	}
+
+	status = nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl,
+				nlmclnt_ops, l_ctx);
+	if (l_ctx)
+		nfs_put_lock_context(l_ctx);
+
+	return status;
 }
 
 static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 91f88bfbbe79..e46cc2be60e2 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5905,7 +5905,7 @@ struct nfs4_unlockdata {
 	struct nfs_locku_args arg;
 	struct nfs_locku_res res;
 	struct nfs4_lock_state *lsp;
-	struct nfs_open_context *ctx;
+	struct nfs_lock_context *l_ctx;
 	struct file_lock fl;
 	struct nfs_server *server;
 	unsigned long timestamp;
@@ -5929,7 +5929,7 @@ static struct nfs4_unlockdata *nfs4_alloc_unlockdata(struct file_lock *fl,
 	p->lsp = lsp;
 	atomic_inc(&lsp->ls_count);
 	/* Ensure we don't close file until we're done freeing locks! */
-	p->ctx = get_nfs_open_context(ctx);
+	p->l_ctx = nfs_get_lock_context(ctx);
 	memcpy(&p->fl, fl, sizeof(p->fl));
 	p->server = NFS_SERVER(inode);
 	return p;
@@ -5940,7 +5940,7 @@ static void nfs4_locku_release_calldata(void *data)
 	struct nfs4_unlockdata *calldata = data;
 	nfs_free_seqid(calldata->arg.seqid);
 	nfs4_put_lock_state(calldata->lsp);
-	put_nfs_open_context(calldata->ctx);
+	nfs_put_lock_context(calldata->l_ctx);
 	kfree(calldata);
 }
 
@@ -5981,6 +5981,10 @@ static void nfs4_locku_prepare(struct rpc_task *task, void *data)
 {
 	struct nfs4_unlockdata *calldata = data;
 
+	if (calldata->fl.fl_flags & FL_CLOSE &&
+		nfs_async_iocounter_wait(task, calldata->l_ctx))
+		return;
+
 	if (nfs_wait_on_sequence(calldata->arg.seqid, task) != 0)
 		goto out_wait;
 	nfs4_stateid_copy(&calldata->arg.stateid, &calldata->lsp->ls_stateid);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/6] locks: Set FL_CLOSE when removing flock locks on close()
  2017-04-06 11:23 ` [PATCH 3/6] locks: Set FL_CLOSE when removing flock locks on close() Benjamin Coddington
@ 2017-04-06 18:17     ` Jeff Layton
  2017-04-08  5:34   ` kbuild test robot
  1 sibling, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-06 18:17 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
> Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks.
> NFS will check for this flag to ensure an unlock is sent in a following
> patch.
> 
> Fuse handles flock and posix locks differently for FL_CLOSE, and so
> requires a fixup to retain the existing behavior for flock.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/fuse/file.c     | 2 +-
>  fs/locks.c         | 2 +-
>  include/linux/fs.h | 2 ++
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 2401c5dabb2a..79e1a3a048a8 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2172,7 +2172,7 @@ static int fuse_setlk(struct file *file, struct file_lock *fl, int flock)
>  	}
>  
>  	/* Unlock on close is handled by the flush method */
> -	if (fl->fl_flags & FL_CLOSE)
> +	if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
>  		return 0;
>  
>  	fuse_lk_fill(&args, file, fl, opcode, pid, flock, &inarg);
> diff --git a/fs/locks.c b/fs/locks.c
> index 26811321d39b..af2031a1fcff 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
>  		.fl_owner = filp,
>  		.fl_pid = current->tgid,
>  		.fl_file = filp,
> -		.fl_flags = FL_FLOCK,
> +		.fl_flags = FL_FLOCK | FL_CLOSE,
>  		.fl_type = F_UNLCK,
>  		.fl_end = OFFSET_MAX,
>  	};
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba074328894..e964ae0a9c29 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -903,6 +903,8 @@ static inline struct file *get_file(struct file *f)
>  #define FL_OFDLCK	1024	/* lock is "owned" by struct file */
>  #define FL_LAYOUT	2048	/* outstanding pNFS layout */
>  
> +#define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
> +
>  /*
>   * Special return value from posix_lock_file() and vfs_lock_file() for
>   * asynchronous locking.


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/6] locks: Set FL_CLOSE when removing flock locks on close()
@ 2017-04-06 18:17     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-06 18:17 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
> Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks.
> NFS will check for this flag to ensure an unlock is sent in a following
> patch.
> 
> Fuse handles flock and posix locks differently for FL_CLOSE, and so
> requires a fixup to retain the existing behavior for flock.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/fuse/file.c     | 2 +-
>  fs/locks.c         | 2 +-
>  include/linux/fs.h | 2 ++
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 2401c5dabb2a..79e1a3a048a8 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2172,7 +2172,7 @@ static int fuse_setlk(struct file *file, struct file_lock *fl, int flock)
>  	}
>  
>  	/* Unlock on close is handled by the flush method */
> -	if (fl->fl_flags & FL_CLOSE)
> +	if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
>  		return 0;
>  
>  	fuse_lk_fill(&args, file, fl, opcode, pid, flock, &inarg);
> diff --git a/fs/locks.c b/fs/locks.c
> index 26811321d39b..af2031a1fcff 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
>  		.fl_owner = filp,
>  		.fl_pid = current->tgid,
>  		.fl_file = filp,
> -		.fl_flags = FL_FLOCK,
> +		.fl_flags = FL_FLOCK | FL_CLOSE,
>  		.fl_type = F_UNLCK,
>  		.fl_end = OFFSET_MAX,
>  	};
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba074328894..e964ae0a9c29 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -903,6 +903,8 @@ static inline struct file *get_file(struct file *f)
>  #define FL_OFDLCK	1024	/* lock is "owned" by struct file */
>  #define FL_LAYOUT	2048	/* outstanding pNFS layout */
>  
> +#define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
> +
>  /*
>   * Special return value from posix_lock_file() and vfs_lock_file() for
>   * asynchronous locking.


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/6] NFS: Add an iocounter wait function for async RPC tasks
  2017-04-06 11:23 ` [PATCH 4/6] NFS: Add an iocounter wait function for async RPC tasks Benjamin Coddington
@ 2017-04-07 10:44     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-07 10:44 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
> By sleeping on a new NFS Unlock-On-Close waitqueue, rpc tasks may wait for
> a lock context's iocounter to reach zero.  The rpc waitqueue is only woken
> when the open_context has the NFS_CONTEXT_UNLOCK flag set in order to
> mitigate spurious wake-ups for any iocounter reaching zero.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/client.c           |  1 +
>  fs/nfs/pagelist.c         | 26 +++++++++++++++++++++++++-
>  include/linux/nfs_fs.h    |  1 +
>  include/linux/nfs_fs_sb.h |  1 +
>  include/linux/nfs_page.h  |  1 +
>  5 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 91a8d610ba0f..c335c6dce285 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -218,6 +218,7 @@ static void nfs_cb_idr_remove_locked(struct nfs_client *clp)
>  static void pnfs_init_server(struct nfs_server *server)
>  {
>  	rpc_init_wait_queue(&server->roc_rpcwaitq, "pNFS ROC");
> +	rpc_init_wait_queue(&server->uoc_rpcwaitq, "NFS UOC");
>  }
>  
>  #else
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 6e629b856a00..d163819e4416 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -115,6 +115,27 @@ nfs_iocounter_wait(struct nfs_lock_context *l_ctx)
>  			TASK_KILLABLE);
>  }
>  
> +bool
> +nfs_async_iocounter_wait(struct rpc_task *task, void *data)
> +{
> +	struct nfs_lock_context *l_ctx = data;
> +	struct inode *inode = d_inode(l_ctx->open_context->dentry);
> +	bool ret = false;
> +
> +	if (atomic_read(&l_ctx->io_count) > 0) {
> +		rpc_sleep_on(&NFS_SERVER(inode)->uoc_rpcwaitq, task, NULL);
> +		ret = true;
> +	}
> +
> +	if (atomic_read(&l_ctx->io_count) == 0) {
> +		rpc_wake_up_queued_task(&NFS_SERVER(inode)->uoc_rpcwaitq, task);
> +		ret = false;
> +	}
> +

nit: Would it be better to do the atomic read once into a local variable
and then compare it afterward?

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(nfs_async_iocounter_wait);
> +
>  /*
>   * nfs_page_group_lock - lock the head of the page group
>   * @req - request in group that is to be locked
> @@ -398,8 +419,11 @@ static void nfs_clear_request(struct nfs_page *req)
>  		req->wb_page = NULL;
>  	}
>  	if (l_ctx != NULL) {
> -		if (atomic_dec_and_test(&l_ctx->io_count))
> +		if (atomic_dec_and_test(&l_ctx->io_count)) {
>  			wake_up_atomic_t(&l_ctx->io_count);
> +			if (test_bit(NFS_CONTEXT_UNLOCK, &ctx->flags))
> +				rpc_wake_up(&NFS_SERVER(d_inode(ctx->dentry))->uoc_rpcwaitq);
> +		}
>  		nfs_put_lock_context(l_ctx);
>  		req->wb_lock_context = NULL;
>  	}
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index f1da8c8dd473..3ce3e42beead 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -76,6 +76,7 @@ struct nfs_open_context {
>  #define NFS_CONTEXT_ERROR_WRITE		(0)
>  #define NFS_CONTEXT_RESEND_WRITES	(1)
>  #define NFS_CONTEXT_BAD			(2)
> +#define NFS_CONTEXT_UNLOCK	(3)
>  	int error;
>  
>  	struct list_head list;
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index b34097c67848..2a70f34dffe8 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -222,6 +222,7 @@ struct nfs_server {
>  	u32			mountd_version;
>  	unsigned short		mountd_port;
>  	unsigned short		mountd_protocol;
> +	struct rpc_wait_queue	uoc_rpcwaitq;
>  };
>  
>  /* Server capabilities */
> diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
> index 957049f72290..d39ebcba52c8 100644
> --- a/include/linux/nfs_page.h
> +++ b/include/linux/nfs_page.h
> @@ -141,6 +141,7 @@ extern int nfs_page_group_lock(struct nfs_page *, bool);
>  extern void nfs_page_group_lock_wait(struct nfs_page *);
>  extern void nfs_page_group_unlock(struct nfs_page *);
>  extern bool nfs_page_group_sync_on_bit(struct nfs_page *, unsigned int);
> +extern bool nfs_async_iocounter_wait(struct rpc_task *, void *);
>  
>  /*
>   * Lock the page of an asynchronous request

Looks reasonable otherwise:

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/6] NFS: Add an iocounter wait function for async RPC tasks
@ 2017-04-07 10:44     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-07 10:44 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
> By sleeping on a new NFS Unlock-On-Close waitqueue, rpc tasks may wait for
> a lock context's iocounter to reach zero.  The rpc waitqueue is only woken
> when the open_context has the NFS_CONTEXT_UNLOCK flag set in order to
> mitigate spurious wake-ups for any iocounter reaching zero.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/client.c           |  1 +
>  fs/nfs/pagelist.c         | 26 +++++++++++++++++++++++++-
>  include/linux/nfs_fs.h    |  1 +
>  include/linux/nfs_fs_sb.h |  1 +
>  include/linux/nfs_page.h  |  1 +
>  5 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 91a8d610ba0f..c335c6dce285 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -218,6 +218,7 @@ static void nfs_cb_idr_remove_locked(struct nfs_client *clp)
>  static void pnfs_init_server(struct nfs_server *server)
>  {
>  	rpc_init_wait_queue(&server->roc_rpcwaitq, "pNFS ROC");
> +	rpc_init_wait_queue(&server->uoc_rpcwaitq, "NFS UOC");
>  }
>  
>  #else
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 6e629b856a00..d163819e4416 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -115,6 +115,27 @@ nfs_iocounter_wait(struct nfs_lock_context *l_ctx)
>  			TASK_KILLABLE);
>  }
>  
> +bool
> +nfs_async_iocounter_wait(struct rpc_task *task, void *data)
> +{
> +	struct nfs_lock_context *l_ctx = data;
> +	struct inode *inode = d_inode(l_ctx->open_context->dentry);
> +	bool ret = false;
> +
> +	if (atomic_read(&l_ctx->io_count) > 0) {
> +		rpc_sleep_on(&NFS_SERVER(inode)->uoc_rpcwaitq, task, NULL);
> +		ret = true;
> +	}
> +
> +	if (atomic_read(&l_ctx->io_count) == 0) {
> +		rpc_wake_up_queued_task(&NFS_SERVER(inode)->uoc_rpcwaitq, task);
> +		ret = false;
> +	}
> +

nit: Would it be better to do the atomic read once into a local variable
and then compare it afterward?

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(nfs_async_iocounter_wait);
> +
>  /*
>   * nfs_page_group_lock - lock the head of the page group
>   * @req - request in group that is to be locked
> @@ -398,8 +419,11 @@ static void nfs_clear_request(struct nfs_page *req)
>  		req->wb_page = NULL;
>  	}
>  	if (l_ctx != NULL) {
> -		if (atomic_dec_and_test(&l_ctx->io_count))
> +		if (atomic_dec_and_test(&l_ctx->io_count)) {
>  			wake_up_atomic_t(&l_ctx->io_count);
> +			if (test_bit(NFS_CONTEXT_UNLOCK, &ctx->flags))
> +				rpc_wake_up(&NFS_SERVER(d_inode(ctx->dentry))->uoc_rpcwaitq);
> +		}
>  		nfs_put_lock_context(l_ctx);
>  		req->wb_lock_context = NULL;
>  	}
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index f1da8c8dd473..3ce3e42beead 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -76,6 +76,7 @@ struct nfs_open_context {
>  #define NFS_CONTEXT_ERROR_WRITE		(0)
>  #define NFS_CONTEXT_RESEND_WRITES	(1)
>  #define NFS_CONTEXT_BAD			(2)
> +#define NFS_CONTEXT_UNLOCK	(3)
>  	int error;
>  
>  	struct list_head list;
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index b34097c67848..2a70f34dffe8 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -222,6 +222,7 @@ struct nfs_server {
>  	u32			mountd_version;
>  	unsigned short		mountd_port;
>  	unsigned short		mountd_protocol;
> +	struct rpc_wait_queue	uoc_rpcwaitq;
>  };
>  
>  /* Server capabilities */
> diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
> index 957049f72290..d39ebcba52c8 100644
> --- a/include/linux/nfs_page.h
> +++ b/include/linux/nfs_page.h
> @@ -141,6 +141,7 @@ extern int nfs_page_group_lock(struct nfs_page *, bool);
>  extern void nfs_page_group_lock_wait(struct nfs_page *);
>  extern void nfs_page_group_unlock(struct nfs_page *);
>  extern bool nfs_page_group_sync_on_bit(struct nfs_page *, unsigned int);
> +extern bool nfs_async_iocounter_wait(struct rpc_task *, void *);
>  
>  /*
>   * Lock the page of an asynchronous request

Looks reasonable otherwise:

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/6] NFS: Add an iocounter wait function for async RPC tasks
  2017-04-07 10:44     ` Jeff Layton
  (?)
@ 2017-04-07 11:26     ` Benjamin Coddington
  -1 siblings, 0 replies; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-07 11:26 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Trond Myklebust, Anna Schumaker, bfields, Miklos Szeredi,
	Alexander Viro, linux-nfs, linux-fsdevel

On 7 Apr 2017, at 6:44, Jeff Layton wrote:

> On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
>> By sleeping on a new NFS Unlock-On-Close waitqueue, rpc tasks may 
>> wait for
>> a lock context's iocounter to reach zero.  The rpc waitqueue is only 
>> woken
>> when the open_context has the NFS_CONTEXT_UNLOCK flag set in order to
>> mitigate spurious wake-ups for any iocounter reaching zero.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>  fs/nfs/client.c           |  1 +
>>  fs/nfs/pagelist.c         | 26 +++++++++++++++++++++++++-
>>  include/linux/nfs_fs.h    |  1 +
>>  include/linux/nfs_fs_sb.h |  1 +
>>  include/linux/nfs_page.h  |  1 +
>>  5 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
>> index 91a8d610ba0f..c335c6dce285 100644
>> --- a/fs/nfs/client.c
>> +++ b/fs/nfs/client.c
>> @@ -218,6 +218,7 @@ static void nfs_cb_idr_remove_locked(struct 
>> nfs_client *clp)
>>  static void pnfs_init_server(struct nfs_server *server)
>>  {
>>  	rpc_init_wait_queue(&server->roc_rpcwaitq, "pNFS ROC");
>> +	rpc_init_wait_queue(&server->uoc_rpcwaitq, "NFS UOC");
>>  }
>>
>>  #else
>> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
>> index 6e629b856a00..d163819e4416 100644
>> --- a/fs/nfs/pagelist.c
>> +++ b/fs/nfs/pagelist.c
>> @@ -115,6 +115,27 @@ nfs_iocounter_wait(struct nfs_lock_context 
>> *l_ctx)
>>  			TASK_KILLABLE);
>>  }
>>
>> +bool
>> +nfs_async_iocounter_wait(struct rpc_task *task, void *data)
>> +{
>> +	struct nfs_lock_context *l_ctx = data;
>> +	struct inode *inode = d_inode(l_ctx->open_context->dentry);
>> +	bool ret = false;
>> +
>> +	if (atomic_read(&l_ctx->io_count) > 0) {
>> +		rpc_sleep_on(&NFS_SERVER(inode)->uoc_rpcwaitq, task, NULL);
>> +		ret = true;
>> +	}
>> +
>> +	if (atomic_read(&l_ctx->io_count) == 0) {
>> +		rpc_wake_up_queued_task(&NFS_SERVER(inode)->uoc_rpcwaitq, task);
>> +		ret = false;
>> +	}
>> +
>
> nit: Would it be better to do the atomic read once into a local 
> variable
> and then compare it afterward?

Reading it twice catches the race where we are about to put the task on 
the
wait queue, but then the iocounter goes to zero and the queue wakes.

>
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(nfs_async_iocounter_wait);
>> +
>>  /*
>>   * nfs_page_group_lock - lock the head of the page group
>>   * @req - request in group that is to be locked
>> @@ -398,8 +419,11 @@ static void nfs_clear_request(struct nfs_page 
>> *req)
>>  		req->wb_page = NULL;
>>  	}
>>  	if (l_ctx != NULL) {
>> -		if (atomic_dec_and_test(&l_ctx->io_count))
>> +		if (atomic_dec_and_test(&l_ctx->io_count)) {
>>  			wake_up_atomic_t(&l_ctx->io_count);
>> +			if (test_bit(NFS_CONTEXT_UNLOCK, &ctx->flags))
>> +				rpc_wake_up(&NFS_SERVER(d_inode(ctx->dentry))->uoc_rpcwaitq);
>> +		}
>>  		nfs_put_lock_context(l_ctx);
>>  		req->wb_lock_context = NULL;
>>  	}
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index f1da8c8dd473..3ce3e42beead 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -76,6 +76,7 @@ struct nfs_open_context {
>>  #define NFS_CONTEXT_ERROR_WRITE		(0)
>>  #define NFS_CONTEXT_RESEND_WRITES	(1)
>>  #define NFS_CONTEXT_BAD			(2)
>> +#define NFS_CONTEXT_UNLOCK	(3)
>>  	int error;
>>
>>  	struct list_head list;
>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
>> index b34097c67848..2a70f34dffe8 100644
>> --- a/include/linux/nfs_fs_sb.h
>> +++ b/include/linux/nfs_fs_sb.h
>> @@ -222,6 +222,7 @@ struct nfs_server {
>>  	u32			mountd_version;
>>  	unsigned short		mountd_port;
>>  	unsigned short		mountd_protocol;
>> +	struct rpc_wait_queue	uoc_rpcwaitq;
>>  };
>>
>>  /* Server capabilities */
>> diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
>> index 957049f72290..d39ebcba52c8 100644
>> --- a/include/linux/nfs_page.h
>> +++ b/include/linux/nfs_page.h
>> @@ -141,6 +141,7 @@ extern int nfs_page_group_lock(struct nfs_page *, 
>> bool);
>>  extern void nfs_page_group_lock_wait(struct nfs_page *);
>>  extern void nfs_page_group_unlock(struct nfs_page *);
>>  extern bool nfs_page_group_sync_on_bit(struct nfs_page *, unsigned 
>> int);
>> +extern bool nfs_async_iocounter_wait(struct rpc_task *, void *);
>>
>>  /*
>>   * Lock the page of an asynchronous request
>
> Looks reasonable otherwise:
>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>

Thanks for the review, and all the help..

Ben

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/6] lockd: Introduce nlmclnt_operations
  2017-04-06 11:23 ` [PATCH 5/6] lockd: Introduce nlmclnt_operations Benjamin Coddington
@ 2017-04-07 12:10     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-07 12:10 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
> NFS would enjoy the ability to modify the behavior of the NLM client's
> unlock RPC task in order to delay the transmission of the unlock until IO
> that was submitted under that lock has completed.  This ability can ensure
> that the NLM client will always complete the transmission of an unlock even
> if the waiting caller has been interrupted with fatal signal.
> 
> A struct nlmclnt_operations and callback data pointer can be passed to
> nlmclnt_proc(). The struct nlmclnt_operations defines three callback
> operations that will be used in a following patch:
> 
> nlmclnt_alloc_call - used to call back after a successful allocation of
> 	a struct nlm_rqst in nlmclnt_proc().
> 
> nlmclnt_unlock_prepare - used to call back during NLM unlock's
> 	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
> 	until this callback returns false.
> 
> nlmclnt_release_call - used to call back when the NLM client's struct
> 	nlm_rqst is freed.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
>  fs/nfs/nfs3proc.c           |  2 +-
>  fs/nfs/proc.c               |  2 +-
>  include/linux/lockd/bind.h  | 11 +++++++++--
>  include/linux/lockd/lockd.h |  2 ++
>  5 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 112952037933..d2744f4960ed 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -150,9 +150,12 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
>   * @host: address of a valid nlm_host context representing the NLM server
>   * @cmd: fcntl-style file lock operation to perform
>   * @fl: address of arguments for the lock operation
> + * @nlmclnt_ops: operations to call back out of nlm
> + * @data: address of data to be sent along with callback


I now get that you're using the presence or absence of the ops pointer
as an indicator for the underlying layers to do the callbacks or not.
That's a little odd, and should probably be clearly documented.

I still think it might be cleaner to put these ops in the host and
handle the condition of whether to run them in the callbacks themselves,
but I can live with what you have here. We can always change that later
if we find it to be problematic.

>   *
>   */
> -int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
> +int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
> +		const struct nlmclnt_operations *nlmclnt_ops, void *data)
>  {
>  	struct nlm_rqst		*call;
>  	int			status;
> @@ -161,6 +164,9 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	if (call == NULL)
>  		return -ENOMEM;
>  
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
> +		nlmclnt_ops->nlmclnt_alloc_call(data);
> +


>  	nlmclnt_locks_init_private(fl, host);
>  	if (!fl->fl_u.nfs_fl.owner) {
>  		/* lockowner allocation has failed */
> @@ -169,6 +175,8 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	}
>  	/* Set up the argument struct */
>  	nlmclnt_setlockargs(call, fl);
> +	call->nlmclnt_ops = nlmclnt_ops;
> +	call->callback_data = data;
>  
>  	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
>  		if (fl->fl_type != F_UNLCK) {
> @@ -216,6 +224,8 @@ void nlmclnt_release_call(struct nlm_rqst *call)
>  {
>  	if (!atomic_dec_and_test(&call->a_count))
>  		return;
> +	if (call->nlmclnt_ops && call->nlmclnt_ops->nlmclnt_release_call)
> +		call->nlmclnt_ops->nlmclnt_release_call(call->callback_data);
>  	nlmclnt_release_host(call->a_host);
>  	nlmclnt_release_lockargs(call);
>  	kfree(call);
> @@ -687,6 +697,19 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
>  	return status;
>  }
>  
> +static void nlmclnt_unlock_prepare(struct rpc_task *task, void *data)
> +{
> +	struct nlm_rqst	*req = data;
> +	const struct nlmclnt_operations *nlmclnt_ops = req->nlmclnt_ops;
> +	bool defer_call = false;
> +
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_unlock_prepare)
> +		defer_call = nlmclnt_ops->nlmclnt_unlock_prepare(task, req->callback_data);
> +
> +	if (!defer_call)
> +		rpc_call_start(task);
> +}
> +
>  static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  {
>  	struct nlm_rqst	*req = data;
> @@ -720,6 +743,7 @@ static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  }
>  
>  static const struct rpc_call_ops nlmclnt_unlock_ops = {
> +	.rpc_call_prepare = nlmclnt_unlock_prepare,
>  	.rpc_call_done = nlmclnt_unlock_callback,
>  	.rpc_release = nlmclnt_rpc_release,
>  };
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index dc925b531f32..086623ab07b1 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -870,7 +870,7 @@ nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
>  }
>  
>  static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
> diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
> index b7bca8303989..fb8cbdf0a980 100644
> --- a/fs/nfs/proc.c
> +++ b/fs/nfs/proc.c
> @@ -638,7 +638,7 @@ nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
>  }
>  
>  /* Helper functions for NFS lock bounds checking */
> diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
> index 140edab64446..00c3c2a62bb5 100644
> --- a/include/linux/lockd/bind.h
> +++ b/include/linux/lockd/bind.h
> @@ -18,6 +18,7 @@
>  
>  /* Dummy declarations */
>  struct svc_rqst;
> +struct rpc_task;
>  
>  /*
>   * This is the set of functions for lockd->nfsd communication
> @@ -52,8 +53,14 @@ struct nlmclnt_initdata {
>  extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
>  extern void	nlmclnt_done(struct nlm_host *host);
>  
> -extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
> -					struct file_lock *fl);

With a new ops struct like the one below, it's good to add some comments
that explain what each of these operations does, any locking
constraints, etc. Laying out the rules now makes it simpler to follow
them later.

> +struct nlmclnt_operations {
> +	void (*nlmclnt_alloc_call)(void *);
> +	bool (*nlmclnt_unlock_prepare)(struct rpc_task*, void *);
> +	void (*nlmclnt_release_call)(void *);
> +};
> +
> +extern int	nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
> +	const struct nlmclnt_operations *nlmclnt_ops, void *data);
>  extern int	lockd_up(struct net *net);
>  extern void	lockd_down(struct net *net);
>  
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index c15373894a42..0c6f777dde53 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -142,6 +142,8 @@ struct nlm_rqst {
>  	struct nlm_block *	a_block;
>  	unsigned int		a_retries;	/* Retry count */
>  	u8			a_owner[NLMCLNT_OHSIZE];
> +	const struct nlmclnt_operations * nlmclnt_ops;
> +	void * callback_data;

nit: please fix the alignment of the struct there. Also, I think these
pointers should have an "a_" in order to follow the convention.

>  };
>  
>  /*

Overall though, I think this looks good. Assuming you fix the nits:

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/6] lockd: Introduce nlmclnt_operations
@ 2017-04-07 12:10     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-07 12:10 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
> NFS would enjoy the ability to modify the behavior of the NLM client's
> unlock RPC task in order to delay the transmission of the unlock until IO
> that was submitted under that lock has completed.  This ability can ensure
> that the NLM client will always complete the transmission of an unlock even
> if the waiting caller has been interrupted with fatal signal.
> 
> A struct nlmclnt_operations and callback data pointer can be passed to
> nlmclnt_proc(). The struct nlmclnt_operations defines three callback
> operations that will be used in a following patch:
> 
> nlmclnt_alloc_call - used to call back after a successful allocation of
> 	a struct nlm_rqst in nlmclnt_proc().
> 
> nlmclnt_unlock_prepare - used to call back during NLM unlock's
> 	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
> 	until this callback returns false.
> 
> nlmclnt_release_call - used to call back when the NLM client's struct
> 	nlm_rqst is freed.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
>  fs/nfs/nfs3proc.c           |  2 +-
>  fs/nfs/proc.c               |  2 +-
>  include/linux/lockd/bind.h  | 11 +++++++++--
>  include/linux/lockd/lockd.h |  2 ++
>  5 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 112952037933..d2744f4960ed 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -150,9 +150,12 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
>   * @host: address of a valid nlm_host context representing the NLM server
>   * @cmd: fcntl-style file lock operation to perform
>   * @fl: address of arguments for the lock operation
> + * @nlmclnt_ops: operations to call back out of nlm
> + * @data: address of data to be sent along with callback


I now get that you're using the presence or absence of the ops pointer
as an indicator for the underlying layers to do the callbacks or not.
That's a little odd, and should probably be clearly documented.

I still think it might be cleaner to put these ops in the host and
handle the condition of whether to run them in the callbacks themselves,
but I can live with what you have here. We can always change that later
if we find it to be problematic.

>   *
>   */
> -int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
> +int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
> +		const struct nlmclnt_operations *nlmclnt_ops, void *data)
>  {
>  	struct nlm_rqst		*call;
>  	int			status;
> @@ -161,6 +164,9 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	if (call == NULL)
>  		return -ENOMEM;
>  
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
> +		nlmclnt_ops->nlmclnt_alloc_call(data);
> +


>  	nlmclnt_locks_init_private(fl, host);
>  	if (!fl->fl_u.nfs_fl.owner) {
>  		/* lockowner allocation has failed */
> @@ -169,6 +175,8 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	}
>  	/* Set up the argument struct */
>  	nlmclnt_setlockargs(call, fl);
> +	call->nlmclnt_ops = nlmclnt_ops;
> +	call->callback_data = data;
>  
>  	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
>  		if (fl->fl_type != F_UNLCK) {
> @@ -216,6 +224,8 @@ void nlmclnt_release_call(struct nlm_rqst *call)
>  {
>  	if (!atomic_dec_and_test(&call->a_count))
>  		return;
> +	if (call->nlmclnt_ops && call->nlmclnt_ops->nlmclnt_release_call)
> +		call->nlmclnt_ops->nlmclnt_release_call(call->callback_data);
>  	nlmclnt_release_host(call->a_host);
>  	nlmclnt_release_lockargs(call);
>  	kfree(call);
> @@ -687,6 +697,19 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
>  	return status;
>  }
>  
> +static void nlmclnt_unlock_prepare(struct rpc_task *task, void *data)
> +{
> +	struct nlm_rqst	*req = data;
> +	const struct nlmclnt_operations *nlmclnt_ops = req->nlmclnt_ops;
> +	bool defer_call = false;
> +
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_unlock_prepare)
> +		defer_call = nlmclnt_ops->nlmclnt_unlock_prepare(task, req->callback_data);
> +
> +	if (!defer_call)
> +		rpc_call_start(task);
> +}
> +
>  static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  {
>  	struct nlm_rqst	*req = data;
> @@ -720,6 +743,7 @@ static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  }
>  
>  static const struct rpc_call_ops nlmclnt_unlock_ops = {
> +	.rpc_call_prepare = nlmclnt_unlock_prepare,
>  	.rpc_call_done = nlmclnt_unlock_callback,
>  	.rpc_release = nlmclnt_rpc_release,
>  };
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index dc925b531f32..086623ab07b1 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -870,7 +870,7 @@ nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
>  }
>  
>  static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
> diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
> index b7bca8303989..fb8cbdf0a980 100644
> --- a/fs/nfs/proc.c
> +++ b/fs/nfs/proc.c
> @@ -638,7 +638,7 @@ nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
>  }
>  
>  /* Helper functions for NFS lock bounds checking */
> diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
> index 140edab64446..00c3c2a62bb5 100644
> --- a/include/linux/lockd/bind.h
> +++ b/include/linux/lockd/bind.h
> @@ -18,6 +18,7 @@
>  
>  /* Dummy declarations */
>  struct svc_rqst;
> +struct rpc_task;
>  
>  /*
>   * This is the set of functions for lockd->nfsd communication
> @@ -52,8 +53,14 @@ struct nlmclnt_initdata {
>  extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
>  extern void	nlmclnt_done(struct nlm_host *host);
>  
> -extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
> -					struct file_lock *fl);

With a new ops struct like the one below, it's good to add some comments
that explain what each of these operations does, any locking
constraints, etc. Laying out the rules now makes it simpler to follow
them later.

> +struct nlmclnt_operations {
> +	void (*nlmclnt_alloc_call)(void *);
> +	bool (*nlmclnt_unlock_prepare)(struct rpc_task*, void *);
> +	void (*nlmclnt_release_call)(void *);
> +};
> +
> +extern int	nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
> +	const struct nlmclnt_operations *nlmclnt_ops, void *data);
>  extern int	lockd_up(struct net *net);
>  extern void	lockd_down(struct net *net);
>  
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index c15373894a42..0c6f777dde53 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -142,6 +142,8 @@ struct nlm_rqst {
>  	struct nlm_block *	a_block;
>  	unsigned int		a_retries;	/* Retry count */
>  	u8			a_owner[NLMCLNT_OHSIZE];
> +	const struct nlmclnt_operations * nlmclnt_ops;
> +	void * callback_data;

nit: please fix the alignment of the struct there. Also, I think these
pointers should have an "a_" in order to follow the convention.

>  };
>  
>  /*

Overall though, I think this looks good. Assuming you fix the nits:

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 6/6] NFS: Always wait for I/O completion before unlock
  2017-04-06 11:23 ` [PATCH 6/6] NFS: Always wait for I/O completion before unlock Benjamin Coddington
@ 2017-04-07 12:22     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-07 12:22 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
> NFS attempts to wait for read and write completion before unlocking in
> order to ensure that the data returned was protected by the lock.  When
> this waiting is interrupted by a signal, the unlock may be skipped, and
> messages similar to the following are seen in the kernel ring buffer:
> 
> [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
> [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 fl_pid=20183
> [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 fl_pid=20185
> 
> For NFSv3, the missing unlock will cause the server to refuse conflicting
> locks indefinitely.  For NFSv4, the leftover lock will be removed by the
> server after the lease timeout.
> 
> This patch fixes this issue by skipping the usual wait in
> nfs_iocounter_wait if the FL_CLOSE flag is set when signaled.  Instead, the
> wait happens in the unlock RPC task on the NFS UOC rpc_waitqueue.
> 
> For NFSv3, use lockd's new nlmclnt_operations along with
> nfs_async_iocounter_wait to defer NLM's unlock task until the lock
> context's iocounter reaches zero.
> 
> For NFSv4, call nfs_async_iocounter_wait() directly from unlock's
> current rpc_call_prepare.
> 

Dare I ask about v2? :)

Hmm actually, it looks like v2 could use the same operations as v3. I
don't see anything protocol-specific there.

> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/file.c     | 10 +++++-----
>  fs/nfs/nfs3proc.c | 38 +++++++++++++++++++++++++++++++++++++-
>  fs/nfs/nfs4proc.c | 10 +++++++---
>  3 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index a490f45df4db..df695f52bb9d 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
>  	if (!IS_ERR(l_ctx)) {
>  		status = nfs_iocounter_wait(l_ctx);
>  		nfs_put_lock_context(l_ctx);
> -		if (status < 0)
> +		/*  NOTE: special case
> +		 * 	If we're signalled while cleaning up locks on process exit, we
> +		 * 	still need to complete the unlock.
> +		 */
> +		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
>  			return status;
>  	}
>  
> -	/* NOTE: special case
> -	 * 	If we're signalled while cleaning up locks on process exit, we
> -	 * 	still need to complete the unlock.
> -	 */
>  	/*
>  	 * Use local locking if mounted with "-onolock" or with appropriate
>  	 * "-olocal_lock="
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index 086623ab07b1..0e21306bfed9 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -865,12 +865,48 @@ static void nfs3_proc_commit_setup(struct nfs_commit_data *data, struct rpc_mess
>  	msg->rpc_proc = &nfs3_procedures[NFS3PROC_COMMIT];
>  }
>  
> +void nfs3_nlm_alloc_call(void *data)
> +{
> +	struct nfs_lock_context *l_ctx = data;
> +	nfs_get_lock_context(l_ctx->open_context);
> +}
> +
> +void nfs3_nlm_release_call(void *data)
> +{
> +	struct nfs_lock_context *l_ctx = data;
> +	nfs_put_lock_context(l_ctx);
> +}
> +
> +const struct nlmclnt_operations nlmclnt_fl_close_lock_ops = {
> +	.nlmclnt_alloc_call = nfs3_nlm_alloc_call,
> +	.nlmclnt_unlock_prepare = nfs_async_iocounter_wait,
> +	.nlmclnt_release_call = nfs3_nlm_release_call,
> +};
> +
>  static int
>  nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
> +	struct nfs_lock_context *l_ctx = NULL;
> +	const struct nlmclnt_operations *nlmclnt_ops = NULL;
> +	int status;
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
> +	if (fl->fl_flags & FL_CLOSE) {
> +		l_ctx = nfs_get_lock_context(nfs_file_open_context(filp));
> +		if (IS_ERR(l_ctx)) {
> +			l_ctx = NULL;
> +		} else {
> +			set_bit(NFS_CONTEXT_UNLOCK, &l_ctx->open_context->flags);
> +			nlmclnt_ops = &nlmclnt_fl_close_lock_ops;

FWIW, I'm not a huge fan of using the pointer to indicate whether to run
the operations or not. IMO, it'd be cleaner to:

1) store the pointer to the operations struct in the nlm_host, pass it a
pointer to it in the nlmclnt_initdata.

2) make the decision to do the operations or not based on the setting of
NFS_CONTEXT_UNLOCK in the callbacks themselves. If it's not set, just
return quickly without doing anything.

> +		}
> +	}
> +
> +	status = nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl,
> +				nlmclnt_ops, l_ctx);
> +	if (l_ctx)
> +		nfs_put_lock_context(l_ctx);
> +
> +	return status;
>  }
>  
>  static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 91f88bfbbe79..e46cc2be60e2 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5905,7 +5905,7 @@ struct nfs4_unlockdata {
>  	struct nfs_locku_args arg;
>  	struct nfs_locku_res res;
>  	struct nfs4_lock_state *lsp;
> -	struct nfs_open_context *ctx;
> +	struct nfs_lock_context *l_ctx;
>  	struct file_lock fl;
>  	struct nfs_server *server;
>  	unsigned long timestamp;
> @@ -5929,7 +5929,7 @@ static struct nfs4_unlockdata *nfs4_alloc_unlockdata(struct file_lock *fl,
>  	p->lsp = lsp;
>  	atomic_inc(&lsp->ls_count);
>  	/* Ensure we don't close file until we're done freeing locks! */
> -	p->ctx = get_nfs_open_context(ctx);
> +	p->l_ctx = nfs_get_lock_context(ctx);
>  	memcpy(&p->fl, fl, sizeof(p->fl));
>  	p->server = NFS_SERVER(inode);
>  	return p;
> @@ -5940,7 +5940,7 @@ static void nfs4_locku_release_calldata(void *data)
>  	struct nfs4_unlockdata *calldata = data;
>  	nfs_free_seqid(calldata->arg.seqid);
>  	nfs4_put_lock_state(calldata->lsp);
> -	put_nfs_open_context(calldata->ctx);
> +	nfs_put_lock_context(calldata->l_ctx);
>  	kfree(calldata);
>  }
>  
> @@ -5981,6 +5981,10 @@ static void nfs4_locku_prepare(struct rpc_task *task, void *data)
>  {
>  	struct nfs4_unlockdata *calldata = data;
>  
> +	if (calldata->fl.fl_flags & FL_CLOSE &&
> +		nfs_async_iocounter_wait(task, calldata->l_ctx))
> +		return;
> +
>  	if (nfs_wait_on_sequence(calldata->arg.seqid, task) != 0)
>  		goto out_wait;
>  	nfs4_stateid_copy(&calldata->arg.stateid, &calldata->lsp->ls_stateid);

-- 
Jeff Layton <jlayton@poochiereds.net>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 6/6] NFS: Always wait for I/O completion before unlock
@ 2017-04-07 12:22     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-07 12:22 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
> NFS attempts to wait for read and write completion before unlocking in
> order to ensure that the data returned was protected by the lock.  When
> this waiting is interrupted by a signal, the unlock may be skipped, and
> messages similar to the following are seen in the kernel ring buffer:
> 
> [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
> [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 fl_pid=20183
> [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 fl_pid=20185
> 
> For NFSv3, the missing unlock will cause the server to refuse conflicting
> locks indefinitely.  For NFSv4, the leftover lock will be removed by the
> server after the lease timeout.
> 
> This patch fixes this issue by skipping the usual wait in
> nfs_iocounter_wait if the FL_CLOSE flag is set when signaled.  Instead, the
> wait happens in the unlock RPC task on the NFS UOC rpc_waitqueue.
> 
> For NFSv3, use lockd's new nlmclnt_operations along with
> nfs_async_iocounter_wait to defer NLM's unlock task until the lock
> context's iocounter reaches zero.
> 
> For NFSv4, call nfs_async_iocounter_wait() directly from unlock's
> current rpc_call_prepare.
> 

Dare I ask about v2? :)

Hmm actually, it looks like v2 could use the same operations as v3. I
don't see anything protocol-specific there.

> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/file.c     | 10 +++++-----
>  fs/nfs/nfs3proc.c | 38 +++++++++++++++++++++++++++++++++++++-
>  fs/nfs/nfs4proc.c | 10 +++++++---
>  3 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index a490f45df4db..df695f52bb9d 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
>  	if (!IS_ERR(l_ctx)) {
>  		status = nfs_iocounter_wait(l_ctx);
>  		nfs_put_lock_context(l_ctx);
> -		if (status < 0)
> +		/*  NOTE: special case
> +		 * 	If we're signalled while cleaning up locks on process exit, we
> +		 * 	still need to complete the unlock.
> +		 */
> +		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
>  			return status;
>  	}
>  
> -	/* NOTE: special case
> -	 * 	If we're signalled while cleaning up locks on process exit, we
> -	 * 	still need to complete the unlock.
> -	 */
>  	/*
>  	 * Use local locking if mounted with "-onolock" or with appropriate
>  	 * "-olocal_lock="
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index 086623ab07b1..0e21306bfed9 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -865,12 +865,48 @@ static void nfs3_proc_commit_setup(struct nfs_commit_data *data, struct rpc_mess
>  	msg->rpc_proc = &nfs3_procedures[NFS3PROC_COMMIT];
>  }
>  
> +void nfs3_nlm_alloc_call(void *data)
> +{
> +	struct nfs_lock_context *l_ctx = data;
> +	nfs_get_lock_context(l_ctx->open_context);
> +}
> +
> +void nfs3_nlm_release_call(void *data)
> +{
> +	struct nfs_lock_context *l_ctx = data;
> +	nfs_put_lock_context(l_ctx);
> +}
> +
> +const struct nlmclnt_operations nlmclnt_fl_close_lock_ops = {
> +	.nlmclnt_alloc_call = nfs3_nlm_alloc_call,
> +	.nlmclnt_unlock_prepare = nfs_async_iocounter_wait,
> +	.nlmclnt_release_call = nfs3_nlm_release_call,
> +};
> +
>  static int
>  nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
> +	struct nfs_lock_context *l_ctx = NULL;
> +	const struct nlmclnt_operations *nlmclnt_ops = NULL;
> +	int status;
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
> +	if (fl->fl_flags & FL_CLOSE) {
> +		l_ctx = nfs_get_lock_context(nfs_file_open_context(filp));
> +		if (IS_ERR(l_ctx)) {
> +			l_ctx = NULL;
> +		} else {
> +			set_bit(NFS_CONTEXT_UNLOCK, &l_ctx->open_context->flags);
> +			nlmclnt_ops = &nlmclnt_fl_close_lock_ops;

FWIW, I'm not a huge fan of using the pointer to indicate whether to run
the operations or not. IMO, it'd be cleaner to:

1) store the pointer to the operations struct in the nlm_host, pass it a
pointer to it in the nlmclnt_initdata.

2) make the decision to do the operations or not based on the setting of
NFS_CONTEXT_UNLOCK in the callbacks themselves. If it's not set, just
return quickly without doing anything.

> +		}
> +	}
> +
> +	status = nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl,
> +				nlmclnt_ops, l_ctx);
> +	if (l_ctx)
> +		nfs_put_lock_context(l_ctx);
> +
> +	return status;
>  }
>  
>  static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 91f88bfbbe79..e46cc2be60e2 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5905,7 +5905,7 @@ struct nfs4_unlockdata {
>  	struct nfs_locku_args arg;
>  	struct nfs_locku_res res;
>  	struct nfs4_lock_state *lsp;
> -	struct nfs_open_context *ctx;
> +	struct nfs_lock_context *l_ctx;
>  	struct file_lock fl;
>  	struct nfs_server *server;
>  	unsigned long timestamp;
> @@ -5929,7 +5929,7 @@ static struct nfs4_unlockdata *nfs4_alloc_unlockdata(struct file_lock *fl,
>  	p->lsp = lsp;
>  	atomic_inc(&lsp->ls_count);
>  	/* Ensure we don't close file until we're done freeing locks! */
> -	p->ctx = get_nfs_open_context(ctx);
> +	p->l_ctx = nfs_get_lock_context(ctx);
>  	memcpy(&p->fl, fl, sizeof(p->fl));
>  	p->server = NFS_SERVER(inode);
>  	return p;
> @@ -5940,7 +5940,7 @@ static void nfs4_locku_release_calldata(void *data)
>  	struct nfs4_unlockdata *calldata = data;
>  	nfs_free_seqid(calldata->arg.seqid);
>  	nfs4_put_lock_state(calldata->lsp);
> -	put_nfs_open_context(calldata->ctx);
> +	nfs_put_lock_context(calldata->l_ctx);
>  	kfree(calldata);
>  }
>  
> @@ -5981,6 +5981,10 @@ static void nfs4_locku_prepare(struct rpc_task *task, void *data)
>  {
>  	struct nfs4_unlockdata *calldata = data;
>  
> +	if (calldata->fl.fl_flags & FL_CLOSE &&
> +		nfs_async_iocounter_wait(task, calldata->l_ctx))
> +		return;
> +
>  	if (nfs_wait_on_sequence(calldata->arg.seqid, task) != 0)
>  		goto out_wait;
>  	nfs4_stateid_copy(&calldata->arg.stateid, &calldata->lsp->ls_stateid);

-- 
Jeff Layton <jlayton@poochiereds.net>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 6/6] NFS: Always wait for I/O completion before unlock
  2017-04-07 12:22     ` Jeff Layton
  (?)
@ 2017-04-07 13:34     ` Benjamin Coddington
  -1 siblings, 0 replies; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-07 13:34 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Trond Myklebust, Anna Schumaker, bfields, Miklos Szeredi,
	Alexander Viro, linux-nfs, linux-fsdevel



On 7 Apr 2017, at 8:22, Jeff Layton wrote:

> On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote:
>> NFS attempts to wait for read and write completion before unlocking 
>> in
>> order to ensure that the data returned was protected by the lock.  
>> When
>> this waiting is interrupted by a signal, the unlock may be skipped, 
>> and
>> messages similar to the following are seen in the kernel ring buffer:
>>
>> [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
>> [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 
>> fl_pid=20183
>> [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 
>> fl_pid=20185
>>
>> For NFSv3, the missing unlock will cause the server to refuse 
>> conflicting
>> locks indefinitely.  For NFSv4, the leftover lock will be removed by 
>> the
>> server after the lease timeout.
>>
>> This patch fixes this issue by skipping the usual wait in
>> nfs_iocounter_wait if the FL_CLOSE flag is set when signaled.  
>> Instead, the
>> wait happens in the unlock RPC task on the NFS UOC rpc_waitqueue.
>>
>> For NFSv3, use lockd's new nlmclnt_operations along with
>> nfs_async_iocounter_wait to defer NLM's unlock task until the lock
>> context's iocounter reaches zero.
>>
>> For NFSv4, call nfs_async_iocounter_wait() directly from unlock's
>> current rpc_call_prepare.
>>
>
> Dare I ask about v2? :)
>
> Hmm actually, it looks like v2 could use the same operations as v3. I
> don't see anything protocol-specific there.
>
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>  fs/nfs/file.c     | 10 +++++-----
>>  fs/nfs/nfs3proc.c | 38 +++++++++++++++++++++++++++++++++++++-
>>  fs/nfs/nfs4proc.c | 10 +++++++---
>>  3 files changed, 49 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>> index a490f45df4db..df695f52bb9d 100644
>> --- a/fs/nfs/file.c
>> +++ b/fs/nfs/file.c
>> @@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct 
>> file_lock *fl, int is_local)
>>  	if (!IS_ERR(l_ctx)) {
>>  		status = nfs_iocounter_wait(l_ctx);
>>  		nfs_put_lock_context(l_ctx);
>> -		if (status < 0)
>> +		/*  NOTE: special case
>> +		 * 	If we're signalled while cleaning up locks on process exit, we
>> +		 * 	still need to complete the unlock.
>> +		 */
>> +		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
>>  			return status;
>>  	}
>>
>> -	/* NOTE: special case
>> -	 * 	If we're signalled while cleaning up locks on process exit, we
>> -	 * 	still need to complete the unlock.
>> -	 */
>>  	/*
>>  	 * Use local locking if mounted with "-onolock" or with appropriate
>>  	 * "-olocal_lock="
>> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
>> index 086623ab07b1..0e21306bfed9 100644
>> --- a/fs/nfs/nfs3proc.c
>> +++ b/fs/nfs/nfs3proc.c
>> @@ -865,12 +865,48 @@ static void nfs3_proc_commit_setup(struct 
>> nfs_commit_data *data, struct rpc_mess
>>  	msg->rpc_proc = &nfs3_procedures[NFS3PROC_COMMIT];
>>  }
>>
>> +void nfs3_nlm_alloc_call(void *data)
>> +{
>> +	struct nfs_lock_context *l_ctx = data;
>> +	nfs_get_lock_context(l_ctx->open_context);
>> +}
>> +
>> +void nfs3_nlm_release_call(void *data)
>> +{
>> +	struct nfs_lock_context *l_ctx = data;
>> +	nfs_put_lock_context(l_ctx);
>> +}
>> +
>> +const struct nlmclnt_operations nlmclnt_fl_close_lock_ops = {
>> +	.nlmclnt_alloc_call = nfs3_nlm_alloc_call,
>> +	.nlmclnt_unlock_prepare = nfs_async_iocounter_wait,
>> +	.nlmclnt_release_call = nfs3_nlm_release_call,
>> +};
>> +
>>  static int
>>  nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>>  {
>>  	struct inode *inode = file_inode(filp);
>> +	struct nfs_lock_context *l_ctx = NULL;
>> +	const struct nlmclnt_operations *nlmclnt_ops = NULL;
>> +	int status;
>>
>> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, 
>> NULL);
>> +	if (fl->fl_flags & FL_CLOSE) {
>> +		l_ctx = nfs_get_lock_context(nfs_file_open_context(filp));
>> +		if (IS_ERR(l_ctx)) {
>> +			l_ctx = NULL;
>> +		} else {
>> +			set_bit(NFS_CONTEXT_UNLOCK, &l_ctx->open_context->flags);
>> +			nlmclnt_ops = &nlmclnt_fl_close_lock_ops;
>
> FWIW, I'm not a huge fan of using the pointer to indicate whether to 
> run
> the operations or not. IMO, it'd be cleaner to:
>
> 1) store the pointer to the operations struct in the nlm_host, pass it 
> a
> pointer to it in the nlmclnt_initdata.
>
> 2) make the decision to do the operations or not based on the setting 
> of
> NFS_CONTEXT_UNLOCK in the callbacks themselves. If it's not set, just
> return quickly without doing anything.

Yes, I can see how that could be a bit neater.  And I can add in the 
same
behavior for v2 while making this change.

I'll work it up that way and send it again.

Ben

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/6] locks: Set FL_CLOSE when removing flock locks on close()
  2017-04-06 11:23 ` [PATCH 3/6] locks: Set FL_CLOSE when removing flock locks on close() Benjamin Coddington
  2017-04-06 18:17     ` Jeff Layton
@ 2017-04-08  5:34   ` kbuild test robot
  1 sibling, 0 replies; 26+ messages in thread
From: kbuild test robot @ 2017-04-08  5:34 UTC (permalink / raw)
  To: Benjamin Coddington
  Cc: kbuild-all, Trond Myklebust, Anna Schumaker, Jeff Layton,
	bfields, Miklos Szeredi, Alexander Viro, linux-nfs,
	linux-fsdevel

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

Hi Benjamin,

[auto build test WARNING on nfs/linux-next]
[also build test WARNING on v4.11-rc5 next-20170407]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Benjamin-Coddington/Skipped-unlocks/20170407-045120
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: x86_64-randconfig-n0-04081158 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   fs/fuse/file.c: In function 'fuse_setlk':
   fs/fuse/file.c:2171:2: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
     if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
     ^
   fs/fuse/file.c:2171:2: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/uapi/linux/fuse.h:121,
                    from fs/fuse/fuse_i.h:12,
                    from fs/fuse/file.c:9:
>> include/linux/compiler.h:163:17: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
      static struct ftrace_branch_data   \
                    ^
   include/linux/compiler.h:158:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^
   fs/fuse/file.c:2171:2: note: in expansion of macro 'if'
     if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
     ^
--
   fs//fuse/file.c: In function 'fuse_setlk':
   fs//fuse/file.c:2171:2: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
     if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
     ^
   fs//fuse/file.c:2171:2: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/uapi/linux/fuse.h:121,
                    from fs//fuse/fuse_i.h:12,
                    from fs//fuse/file.c:9:
>> include/linux/compiler.h:163:17: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
      static struct ftrace_branch_data   \
                    ^
   include/linux/compiler.h:158:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^
   fs//fuse/file.c:2171:2: note: in expansion of macro 'if'
     if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
     ^

vim +163 include/linux/compiler.h

d45ae1f7 Steven Rostedt (VMware  2017-01-17  147) #  define likely(x)	(__branch_check__(x, 1, __builtin_constant_p(x)))
1f0d69a9 Steven Rostedt          2008-11-12  148  # endif
1f0d69a9 Steven Rostedt          2008-11-12  149  # ifndef unlikely
d45ae1f7 Steven Rostedt (VMware  2017-01-17  150) #  define unlikely(x)	(__branch_check__(x, 0, __builtin_constant_p(x)))
1f0d69a9 Steven Rostedt          2008-11-12  151  # endif
2bcd521a Steven Rostedt          2008-11-21  152  
2bcd521a Steven Rostedt          2008-11-21  153  #ifdef CONFIG_PROFILE_ALL_BRANCHES
2bcd521a Steven Rostedt          2008-11-21  154  /*
2bcd521a Steven Rostedt          2008-11-21  155   * "Define 'is'", Bill Clinton
2bcd521a Steven Rostedt          2008-11-21  156   * "Define 'if'", Steven Rostedt
2bcd521a Steven Rostedt          2008-11-21  157   */
ab3c9c68 Linus Torvalds          2009-04-07  158  #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
ab3c9c68 Linus Torvalds          2009-04-07  159  #define __trace_if(cond) \
b33c8ff4 Arnd Bergmann           2016-02-12  160  	if (__builtin_constant_p(!!(cond)) ? !!(cond) :			\
2bcd521a Steven Rostedt          2008-11-21  161  	({								\
2bcd521a Steven Rostedt          2008-11-21  162  		int ______r;						\
2bcd521a Steven Rostedt          2008-11-21 @163  		static struct ftrace_branch_data			\
2bcd521a Steven Rostedt          2008-11-21  164  			__attribute__((__aligned__(4)))			\
2bcd521a Steven Rostedt          2008-11-21  165  			__attribute__((section("_ftrace_branch")))	\
2bcd521a Steven Rostedt          2008-11-21  166  			______f = {					\
2bcd521a Steven Rostedt          2008-11-21  167  				.func = __func__,			\
2bcd521a Steven Rostedt          2008-11-21  168  				.file = __FILE__,			\
2bcd521a Steven Rostedt          2008-11-21  169  				.line = __LINE__,			\
2bcd521a Steven Rostedt          2008-11-21  170  			};						\
2bcd521a Steven Rostedt          2008-11-21  171  		______r = !!(cond);					\

:::::: The code at line 163 was first introduced by commit
:::::: 2bcd521a684cc94befbe2ce7d5b613c841b0d304 trace: profile all if conditionals

:::::: TO: Steven Rostedt <srostedt@redhat.com>
:::::: CC: Ingo Molnar <mingo@elte.hu>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26517 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/6] lockd: Introduce nlmclnt_operations
  2017-04-11 16:50 ` [PATCH 5/6] lockd: Introduce nlmclnt_operations Benjamin Coddington
@ 2017-04-21 12:25     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-21 12:25 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Tue, 2017-04-11 at 12:50 -0400, Benjamin Coddington wrote:
> NFS would enjoy the ability to modify the behavior of the NLM client's
> unlock RPC task in order to delay the transmission of the unlock until IO
> that was submitted under that lock has completed.  This ability can ensure
> that the NLM client will always complete the transmission of an unlock even
> if the waiting caller has been interrupted with fatal signal.
> 
> For this purpose, a pointer to a struct nlmclnt_operations can be assigned
> in a nfs_module's nfs_rpc_ops that will install those nlmclnt_operations on
> the nlm_host.  The struct nlmclnt_operations defines three callback
> operations that will be used in a following patch:
> 
> nlmclnt_alloc_call - used to call back after a successful allocation of
> 	a struct nlm_rqst in nlmclnt_proc().
> 
> nlmclnt_unlock_prepare - used to call back during NLM unlock's
> 	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
> 	until this callback returns false.
> 
> nlmclnt_release_call - used to call back when the NLM client's struct
> 	nlm_rqst is freed.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/lockd/clntlock.c         |  1 +
>  fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
>  fs/nfs/client.c             |  1 +
>  fs/nfs/nfs3proc.c           |  2 +-
>  fs/nfs/proc.c               |  2 +-
>  include/linux/lockd/bind.h  | 24 ++++++++++++++++++++++--
>  include/linux/lockd/lockd.h |  2 ++
>  include/linux/nfs_xdr.h     |  1 +
>  8 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
> index 41e491b8e5d7..27d577dbe51a 100644
> --- a/fs/lockd/clntlock.c
> +++ b/fs/lockd/clntlock.c
> @@ -69,6 +69,7 @@ struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init)
>  	if (host->h_rpcclnt == NULL && nlm_bind_host(host) == NULL)
>  		goto out_nobind;
>  
> +	host->h_nlmclnt_ops = nlm_init->nlmclnt_ops;
>  	return host;
>  out_nobind:
>  	nlmclnt_release_host(host);
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 112952037933..066ac313ae5c 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -150,17 +150,22 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
>   * @host: address of a valid nlm_host context representing the NLM server
>   * @cmd: fcntl-style file lock operation to perform
>   * @fl: address of arguments for the lock operation
> + * @data: address of data to be sent to callback operations
>   *
>   */
> -int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
> +int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *data)
>  {
>  	struct nlm_rqst		*call;
>  	int			status;
> +	const struct nlmclnt_operations *nlmclnt_ops = host->h_nlmclnt_ops;
>  
>  	call = nlm_alloc_call(host);
>  	if (call == NULL)
>  		return -ENOMEM;
>  
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
> +		nlmclnt_ops->nlmclnt_alloc_call(data);
> +
>  	nlmclnt_locks_init_private(fl, host);
>  	if (!fl->fl_u.nfs_fl.owner) {
>  		/* lockowner allocation has failed */
> @@ -169,6 +174,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	}
>  	/* Set up the argument struct */
>  	nlmclnt_setlockargs(call, fl);
> +	call->a_callback_data = data;
>  
>  	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
>  		if (fl->fl_type != F_UNLCK) {
> @@ -214,8 +220,12 @@ struct nlm_rqst *nlm_alloc_call(struct nlm_host *host)
>  
>  void nlmclnt_release_call(struct nlm_rqst *call)
>  {
> +	const struct nlmclnt_operations *nlmclnt_ops = call->a_host->h_nlmclnt_ops;
> +
>  	if (!atomic_dec_and_test(&call->a_count))
>  		return;
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_release_call)
> +		nlmclnt_ops->nlmclnt_release_call(call->a_callback_data);
>  	nlmclnt_release_host(call->a_host);
>  	nlmclnt_release_lockargs(call);
>  	kfree(call);
> @@ -687,6 +697,19 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
>  	return status;
>  }
>  
> +static void nlmclnt_unlock_prepare(struct rpc_task *task, void *data)
> +{
> +	struct nlm_rqst	*req = data;
> +	const struct nlmclnt_operations *nlmclnt_ops = req->a_host->h_nlmclnt_ops;
> +	bool defer_call = false;
> +
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_unlock_prepare)
> +		defer_call = nlmclnt_ops->nlmclnt_unlock_prepare(task, req->a_callback_data);
> +
> +	if (!defer_call)
> +		rpc_call_start(task);
> +}
> +
>  static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  {
>  	struct nlm_rqst	*req = data;
> @@ -720,6 +743,7 @@ static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  }
>  
>  static const struct rpc_call_ops nlmclnt_unlock_ops = {
> +	.rpc_call_prepare = nlmclnt_unlock_prepare,
>  	.rpc_call_done = nlmclnt_unlock_callback,
>  	.rpc_release = nlmclnt_rpc_release,
>  };
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index c335c6dce285..f4c022e57541 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -536,6 +536,7 @@ static int nfs_start_lockd(struct nfs_server *server)
>  		.noresvport	= server->flags & NFS_MOUNT_NORESVPORT ?
>  					1 : 0,
>  		.net		= clp->cl_net,
> +		.nlmclnt_ops 	= clp->cl_nfs_mod->rpc_ops->nlmclnt_ops,
>  	};
>  
>  	if (nlm_init.nfs_version > 3)
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index dc925b531f32..03b3c3de28f1 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -870,7 +870,7 @@ nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL);
>  }
>  
>  static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
> diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
> index b7bca8303989..9872cf676a50 100644
> --- a/fs/nfs/proc.c
> +++ b/fs/nfs/proc.c
> @@ -638,7 +638,7 @@ nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL);
>  }
>  
>  /* Helper functions for NFS lock bounds checking */
> diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
> index 140edab64446..05728396a1a1 100644
> --- a/include/linux/lockd/bind.h
> +++ b/include/linux/lockd/bind.h
> @@ -18,6 +18,7 @@
>  
>  /* Dummy declarations */
>  struct svc_rqst;
> +struct rpc_task;
>  
>  /*
>   * This is the set of functions for lockd->nfsd communication
> @@ -43,6 +44,7 @@ struct nlmclnt_initdata {
>  	u32			nfs_version;
>  	int			noresvport;
>  	struct net		*net;
> +	const struct nlmclnt_operations	*nlmclnt_ops;
>  };
>  
>  /*
> @@ -52,8 +54,26 @@ struct nlmclnt_initdata {
>  extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
>  extern void	nlmclnt_done(struct nlm_host *host);
>  
> -extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
> -					struct file_lock *fl);
> +/*
> + * NLM client operations provide a means to modify RPC processing of NLM
> + * requests.  Callbacks receive a pointer to data passed into the call to
> + * nlmclnt_proc().
> + */
> +struct nlmclnt_operations {
> +	/* Called on successful allocation of nlm_rqst, use for allocation or
> +	 * reference counting. */
> +	void (*nlmclnt_alloc_call)(void *);
> +
> +	/* Called in rpc_task_prepare for unlock.  A return value of true
> +	 * indicates the callback has put the task to sleep on a waitqueue
> +	 * and NLM should not call rpc_call_start(). */
> +	bool (*nlmclnt_unlock_prepare)(struct rpc_task*, void *);
> +
> +	/* Called when the nlm_rqst is freed, callbacks should clean up here */
> +	void (*nlmclnt_release_call)(void *);
> +};
> +
> +extern int	nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *data);
>  extern int	lockd_up(struct net *net);
>  extern void	lockd_down(struct net *net);
>  
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index c15373894a42..01c38992150d 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -69,6 +69,7 @@ struct nlm_host {
>  	char			*h_addrbuf;	/* address eyecatcher */
>  	struct net		*net;		/* host net */
>  	char			nodename[UNX_MAXNODENAME + 1];
> +	const struct nlmclnt_operations	*h_nlmclnt_ops;	/* Callback ops for NLM users */
>  };
>  
>  /*
> @@ -142,6 +143,7 @@ struct nlm_rqst {
>  	struct nlm_block *	a_block;
>  	unsigned int		a_retries;	/* Retry count */
>  	u8			a_owner[NLMCLNT_OHSIZE];
> +	void *	a_callback_data; /* sent to nlmclnt_operations callbacks */
>  };
>  
>  /*
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 348f7c158084..3279ab2e1fee 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1550,6 +1550,7 @@ struct nfs_rpc_ops {
>  	const struct inode_operations *dir_inode_ops;
>  	const struct inode_operations *file_inode_ops;
>  	const struct file_operations *file_ops;
> +	const struct nlmclnt_operations *nlmclnt_ops;
>  
>  	int	(*getroot) (struct nfs_server *, struct nfs_fh *,
>  			    struct nfs_fsinfo *);

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/6] lockd: Introduce nlmclnt_operations
@ 2017-04-21 12:25     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-21 12:25 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Tue, 2017-04-11 at 12:50 -0400, Benjamin Coddington wrote:
> NFS would enjoy the ability to modify the behavior of the NLM client's
> unlock RPC task in order to delay the transmission of the unlock until IO
> that was submitted under that lock has completed.  This ability can ensure
> that the NLM client will always complete the transmission of an unlock even
> if the waiting caller has been interrupted with fatal signal.
> 
> For this purpose, a pointer to a struct nlmclnt_operations can be assigned
> in a nfs_module's nfs_rpc_ops that will install those nlmclnt_operations on
> the nlm_host.  The struct nlmclnt_operations defines three callback
> operations that will be used in a following patch:
> 
> nlmclnt_alloc_call - used to call back after a successful allocation of
> 	a struct nlm_rqst in nlmclnt_proc().
> 
> nlmclnt_unlock_prepare - used to call back during NLM unlock's
> 	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
> 	until this callback returns false.
> 
> nlmclnt_release_call - used to call back when the NLM client's struct
> 	nlm_rqst is freed.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/lockd/clntlock.c         |  1 +
>  fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
>  fs/nfs/client.c             |  1 +
>  fs/nfs/nfs3proc.c           |  2 +-
>  fs/nfs/proc.c               |  2 +-
>  include/linux/lockd/bind.h  | 24 ++++++++++++++++++++++--
>  include/linux/lockd/lockd.h |  2 ++
>  include/linux/nfs_xdr.h     |  1 +
>  8 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
> index 41e491b8e5d7..27d577dbe51a 100644
> --- a/fs/lockd/clntlock.c
> +++ b/fs/lockd/clntlock.c
> @@ -69,6 +69,7 @@ struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init)
>  	if (host->h_rpcclnt == NULL && nlm_bind_host(host) == NULL)
>  		goto out_nobind;
>  
> +	host->h_nlmclnt_ops = nlm_init->nlmclnt_ops;
>  	return host;
>  out_nobind:
>  	nlmclnt_release_host(host);
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 112952037933..066ac313ae5c 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -150,17 +150,22 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
>   * @host: address of a valid nlm_host context representing the NLM server
>   * @cmd: fcntl-style file lock operation to perform
>   * @fl: address of arguments for the lock operation
> + * @data: address of data to be sent to callback operations
>   *
>   */
> -int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
> +int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *data)
>  {
>  	struct nlm_rqst		*call;
>  	int			status;
> +	const struct nlmclnt_operations *nlmclnt_ops = host->h_nlmclnt_ops;
>  
>  	call = nlm_alloc_call(host);
>  	if (call == NULL)
>  		return -ENOMEM;
>  
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
> +		nlmclnt_ops->nlmclnt_alloc_call(data);
> +
>  	nlmclnt_locks_init_private(fl, host);
>  	if (!fl->fl_u.nfs_fl.owner) {
>  		/* lockowner allocation has failed */
> @@ -169,6 +174,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	}
>  	/* Set up the argument struct */
>  	nlmclnt_setlockargs(call, fl);
> +	call->a_callback_data = data;
>  
>  	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
>  		if (fl->fl_type != F_UNLCK) {
> @@ -214,8 +220,12 @@ struct nlm_rqst *nlm_alloc_call(struct nlm_host *host)
>  
>  void nlmclnt_release_call(struct nlm_rqst *call)
>  {
> +	const struct nlmclnt_operations *nlmclnt_ops = call->a_host->h_nlmclnt_ops;
> +
>  	if (!atomic_dec_and_test(&call->a_count))
>  		return;
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_release_call)
> +		nlmclnt_ops->nlmclnt_release_call(call->a_callback_data);
>  	nlmclnt_release_host(call->a_host);
>  	nlmclnt_release_lockargs(call);
>  	kfree(call);
> @@ -687,6 +697,19 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
>  	return status;
>  }
>  
> +static void nlmclnt_unlock_prepare(struct rpc_task *task, void *data)
> +{
> +	struct nlm_rqst	*req = data;
> +	const struct nlmclnt_operations *nlmclnt_ops = req->a_host->h_nlmclnt_ops;
> +	bool defer_call = false;
> +
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_unlock_prepare)
> +		defer_call = nlmclnt_ops->nlmclnt_unlock_prepare(task, req->a_callback_data);
> +
> +	if (!defer_call)
> +		rpc_call_start(task);
> +}
> +
>  static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  {
>  	struct nlm_rqst	*req = data;
> @@ -720,6 +743,7 @@ static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  }
>  
>  static const struct rpc_call_ops nlmclnt_unlock_ops = {
> +	.rpc_call_prepare = nlmclnt_unlock_prepare,
>  	.rpc_call_done = nlmclnt_unlock_callback,
>  	.rpc_release = nlmclnt_rpc_release,
>  };
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index c335c6dce285..f4c022e57541 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -536,6 +536,7 @@ static int nfs_start_lockd(struct nfs_server *server)
>  		.noresvport	= server->flags & NFS_MOUNT_NORESVPORT ?
>  					1 : 0,
>  		.net		= clp->cl_net,
> +		.nlmclnt_ops 	= clp->cl_nfs_mod->rpc_ops->nlmclnt_ops,
>  	};
>  
>  	if (nlm_init.nfs_version > 3)
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index dc925b531f32..03b3c3de28f1 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -870,7 +870,7 @@ nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL);
>  }
>  
>  static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
> diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
> index b7bca8303989..9872cf676a50 100644
> --- a/fs/nfs/proc.c
> +++ b/fs/nfs/proc.c
> @@ -638,7 +638,7 @@ nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL);
>  }
>  
>  /* Helper functions for NFS lock bounds checking */
> diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
> index 140edab64446..05728396a1a1 100644
> --- a/include/linux/lockd/bind.h
> +++ b/include/linux/lockd/bind.h
> @@ -18,6 +18,7 @@
>  
>  /* Dummy declarations */
>  struct svc_rqst;
> +struct rpc_task;
>  
>  /*
>   * This is the set of functions for lockd->nfsd communication
> @@ -43,6 +44,7 @@ struct nlmclnt_initdata {
>  	u32			nfs_version;
>  	int			noresvport;
>  	struct net		*net;
> +	const struct nlmclnt_operations	*nlmclnt_ops;
>  };
>  
>  /*
> @@ -52,8 +54,26 @@ struct nlmclnt_initdata {
>  extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
>  extern void	nlmclnt_done(struct nlm_host *host);
>  
> -extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
> -					struct file_lock *fl);
> +/*
> + * NLM client operations provide a means to modify RPC processing of NLM
> + * requests.  Callbacks receive a pointer to data passed into the call to
> + * nlmclnt_proc().
> + */
> +struct nlmclnt_operations {
> +	/* Called on successful allocation of nlm_rqst, use for allocation or
> +	 * reference counting. */
> +	void (*nlmclnt_alloc_call)(void *);
> +
> +	/* Called in rpc_task_prepare for unlock.  A return value of true
> +	 * indicates the callback has put the task to sleep on a waitqueue
> +	 * and NLM should not call rpc_call_start(). */
> +	bool (*nlmclnt_unlock_prepare)(struct rpc_task*, void *);
> +
> +	/* Called when the nlm_rqst is freed, callbacks should clean up here */
> +	void (*nlmclnt_release_call)(void *);
> +};
> +
> +extern int	nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *data);
>  extern int	lockd_up(struct net *net);
>  extern void	lockd_down(struct net *net);
>  
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index c15373894a42..01c38992150d 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -69,6 +69,7 @@ struct nlm_host {
>  	char			*h_addrbuf;	/* address eyecatcher */
>  	struct net		*net;		/* host net */
>  	char			nodename[UNX_MAXNODENAME + 1];
> +	const struct nlmclnt_operations	*h_nlmclnt_ops;	/* Callback ops for NLM users */
>  };
>  
>  /*
> @@ -142,6 +143,7 @@ struct nlm_rqst {
>  	struct nlm_block *	a_block;
>  	unsigned int		a_retries;	/* Retry count */
>  	u8			a_owner[NLMCLNT_OHSIZE];
> +	void *	a_callback_data; /* sent to nlmclnt_operations callbacks */
>  };
>  
>  /*
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 348f7c158084..3279ab2e1fee 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1550,6 +1550,7 @@ struct nfs_rpc_ops {
>  	const struct inode_operations *dir_inode_ops;
>  	const struct inode_operations *file_inode_ops;
>  	const struct file_operations *file_ops;
> +	const struct nlmclnt_operations *nlmclnt_ops;
>  
>  	int	(*getroot) (struct nfs_server *, struct nfs_fh *,
>  			    struct nfs_fsinfo *);

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 5/6] lockd: Introduce nlmclnt_operations
  2017-04-11 16:50 [PATCH v6 0/6] Skipped unlocks Benjamin Coddington
@ 2017-04-11 16:50 ` Benjamin Coddington
  2017-04-21 12:25     ` Jeff Layton
  0 siblings, 1 reply; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-11 16:50 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, Jeff Layton, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

NFS would enjoy the ability to modify the behavior of the NLM client's
unlock RPC task in order to delay the transmission of the unlock until IO
that was submitted under that lock has completed.  This ability can ensure
that the NLM client will always complete the transmission of an unlock even
if the waiting caller has been interrupted with fatal signal.

For this purpose, a pointer to a struct nlmclnt_operations can be assigned
in a nfs_module's nfs_rpc_ops that will install those nlmclnt_operations on
the nlm_host.  The struct nlmclnt_operations defines three callback
operations that will be used in a following patch:

nlmclnt_alloc_call - used to call back after a successful allocation of
	a struct nlm_rqst in nlmclnt_proc().

nlmclnt_unlock_prepare - used to call back during NLM unlock's
	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
	until this callback returns false.

nlmclnt_release_call - used to call back when the NLM client's struct
	nlm_rqst is freed.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/lockd/clntlock.c         |  1 +
 fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
 fs/nfs/client.c             |  1 +
 fs/nfs/nfs3proc.c           |  2 +-
 fs/nfs/proc.c               |  2 +-
 include/linux/lockd/bind.h  | 24 ++++++++++++++++++++++--
 include/linux/lockd/lockd.h |  2 ++
 include/linux/nfs_xdr.h     |  1 +
 8 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index 41e491b8e5d7..27d577dbe51a 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -69,6 +69,7 @@ struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init)
 	if (host->h_rpcclnt == NULL && nlm_bind_host(host) == NULL)
 		goto out_nobind;
 
+	host->h_nlmclnt_ops = nlm_init->nlmclnt_ops;
 	return host;
 out_nobind:
 	nlmclnt_release_host(host);
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 112952037933..066ac313ae5c 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -150,17 +150,22 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
  * @host: address of a valid nlm_host context representing the NLM server
  * @cmd: fcntl-style file lock operation to perform
  * @fl: address of arguments for the lock operation
+ * @data: address of data to be sent to callback operations
  *
  */
-int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
+int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *data)
 {
 	struct nlm_rqst		*call;
 	int			status;
+	const struct nlmclnt_operations *nlmclnt_ops = host->h_nlmclnt_ops;
 
 	call = nlm_alloc_call(host);
 	if (call == NULL)
 		return -ENOMEM;
 
+	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
+		nlmclnt_ops->nlmclnt_alloc_call(data);
+
 	nlmclnt_locks_init_private(fl, host);
 	if (!fl->fl_u.nfs_fl.owner) {
 		/* lockowner allocation has failed */
@@ -169,6 +174,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
 	}
 	/* Set up the argument struct */
 	nlmclnt_setlockargs(call, fl);
+	call->a_callback_data = data;
 
 	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
 		if (fl->fl_type != F_UNLCK) {
@@ -214,8 +220,12 @@ struct nlm_rqst *nlm_alloc_call(struct nlm_host *host)
 
 void nlmclnt_release_call(struct nlm_rqst *call)
 {
+	const struct nlmclnt_operations *nlmclnt_ops = call->a_host->h_nlmclnt_ops;
+
 	if (!atomic_dec_and_test(&call->a_count))
 		return;
+	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_release_call)
+		nlmclnt_ops->nlmclnt_release_call(call->a_callback_data);
 	nlmclnt_release_host(call->a_host);
 	nlmclnt_release_lockargs(call);
 	kfree(call);
@@ -687,6 +697,19 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
 	return status;
 }
 
+static void nlmclnt_unlock_prepare(struct rpc_task *task, void *data)
+{
+	struct nlm_rqst	*req = data;
+	const struct nlmclnt_operations *nlmclnt_ops = req->a_host->h_nlmclnt_ops;
+	bool defer_call = false;
+
+	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_unlock_prepare)
+		defer_call = nlmclnt_ops->nlmclnt_unlock_prepare(task, req->a_callback_data);
+
+	if (!defer_call)
+		rpc_call_start(task);
+}
+
 static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
 {
 	struct nlm_rqst	*req = data;
@@ -720,6 +743,7 @@ static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
 }
 
 static const struct rpc_call_ops nlmclnt_unlock_ops = {
+	.rpc_call_prepare = nlmclnt_unlock_prepare,
 	.rpc_call_done = nlmclnt_unlock_callback,
 	.rpc_release = nlmclnt_rpc_release,
 };
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index c335c6dce285..f4c022e57541 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -536,6 +536,7 @@ static int nfs_start_lockd(struct nfs_server *server)
 		.noresvport	= server->flags & NFS_MOUNT_NORESVPORT ?
 					1 : 0,
 		.net		= clp->cl_net,
+		.nlmclnt_ops 	= clp->cl_nfs_mod->rpc_ops->nlmclnt_ops,
 	};
 
 	if (nlm_init.nfs_version > 3)
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index dc925b531f32..03b3c3de28f1 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -870,7 +870,7 @@ nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = file_inode(filp);
 
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL);
 }
 
 static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index b7bca8303989..9872cf676a50 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -638,7 +638,7 @@ nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = file_inode(filp);
 
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL);
 }
 
 /* Helper functions for NFS lock bounds checking */
diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
index 140edab64446..05728396a1a1 100644
--- a/include/linux/lockd/bind.h
+++ b/include/linux/lockd/bind.h
@@ -18,6 +18,7 @@
 
 /* Dummy declarations */
 struct svc_rqst;
+struct rpc_task;
 
 /*
  * This is the set of functions for lockd->nfsd communication
@@ -43,6 +44,7 @@ struct nlmclnt_initdata {
 	u32			nfs_version;
 	int			noresvport;
 	struct net		*net;
+	const struct nlmclnt_operations	*nlmclnt_ops;
 };
 
 /*
@@ -52,8 +54,26 @@ struct nlmclnt_initdata {
 extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
 extern void	nlmclnt_done(struct nlm_host *host);
 
-extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
-					struct file_lock *fl);
+/*
+ * NLM client operations provide a means to modify RPC processing of NLM
+ * requests.  Callbacks receive a pointer to data passed into the call to
+ * nlmclnt_proc().
+ */
+struct nlmclnt_operations {
+	/* Called on successful allocation of nlm_rqst, use for allocation or
+	 * reference counting. */
+	void (*nlmclnt_alloc_call)(void *);
+
+	/* Called in rpc_task_prepare for unlock.  A return value of true
+	 * indicates the callback has put the task to sleep on a waitqueue
+	 * and NLM should not call rpc_call_start(). */
+	bool (*nlmclnt_unlock_prepare)(struct rpc_task*, void *);
+
+	/* Called when the nlm_rqst is freed, callbacks should clean up here */
+	void (*nlmclnt_release_call)(void *);
+};
+
+extern int	nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *data);
 extern int	lockd_up(struct net *net);
 extern void	lockd_down(struct net *net);
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index c15373894a42..01c38992150d 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -69,6 +69,7 @@ struct nlm_host {
 	char			*h_addrbuf;	/* address eyecatcher */
 	struct net		*net;		/* host net */
 	char			nodename[UNX_MAXNODENAME + 1];
+	const struct nlmclnt_operations	*h_nlmclnt_ops;	/* Callback ops for NLM users */
 };
 
 /*
@@ -142,6 +143,7 @@ struct nlm_rqst {
 	struct nlm_block *	a_block;
 	unsigned int		a_retries;	/* Retry count */
 	u8			a_owner[NLMCLNT_OHSIZE];
+	void *	a_callback_data; /* sent to nlmclnt_operations callbacks */
 };
 
 /*
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 348f7c158084..3279ab2e1fee 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1550,6 +1550,7 @@ struct nfs_rpc_ops {
 	const struct inode_operations *dir_inode_ops;
 	const struct inode_operations *file_inode_ops;
 	const struct file_operations *file_ops;
+	const struct nlmclnt_operations *nlmclnt_ops;
 
 	int	(*getroot) (struct nfs_server *, struct nfs_fh *,
 			    struct nfs_fsinfo *);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/6] lockd: Introduce nlmclnt_operations
  2017-04-03  9:40     ` Benjamin Coddington
@ 2017-04-06 11:19       ` Benjamin Coddington
  0 siblings, 0 replies; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-06 11:19 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Trond Myklebust, Anna Schumaker, bfields, Miklos Szeredi,
	Alexander Viro, linux-nfs, linux-fsdevel

On 3 Apr 2017, at 5:40, Benjamin Coddington wrote:

> On 1 Apr 2017, at 10:05, Jeff Layton wrote:
>
>> On Fri, 2017-03-31 at 23:16 -0400, Benjamin Coddington wrote:
>>> NFS would enjoy the ability to modify the behavior of the NLM 
>>> client's
>>> unlock RPC task in order to delay the transmission of the unlock 
>>> until IO
>>> that was submitted under that lock has completed.  This ability can 
>>> ensure
>>> that the NLM client will always complete the transmission of an 
>>> unlock even
>>> if the waiting caller has been interrupted with fatal signal.
>>>
>>> A struct nlmclnt_operations and callback data pointer can be passed 
>>> to
>>> nlmclnt_proc(). The struct nlmclnt_operations defines three callback
>>> operations that will be used in a following patch:
>>>
>>> nlmclnt_alloc_call - used to call back after a successful allocation 
>>> of
>>> 	a struct nlm_rqst in nlmclnt_proc().
>>>
>>> nlmclnt_unlock_prepare - used to call back during NLM unlock's
>>> 	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
>>> 	until this callback returns false.
>>>
>>> nlmclnt_release_call - used to call back when the NLM client's 
>>> struct
>>> 	nlm_rqst is freed.
>>>
>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>>> ---
>>>  fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
>>>  fs/nfs/nfs3proc.c           |  2 +-
>>>  fs/nfs/proc.c               |  2 +-
>>>  include/linux/lockd/bind.h  | 11 +++++++++--
>>>  include/linux/lockd/lockd.h |  2 ++
>>>  5 files changed, 38 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
>>> index 112952037933..d2744f4960ed 100644
>>> --- a/fs/lockd/clntproc.c
>>> +++ b/fs/lockd/clntproc.c
>>> @@ -150,9 +150,12 @@ static void nlmclnt_release_lockargs(struct 
>>> nlm_rqst *req)
>>>   * @host: address of a valid nlm_host context representing the NLM 
>>> server
>>>   * @cmd: fcntl-style file lock operation to perform
>>>   * @fl: address of arguments for the lock operation
>>> + * @nlmclnt_ops: operations to call back out of nlm
>>> + * @data: address of data to be sent along with callback
>>>   *
>>>   */
>>> -int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock 
>>> *fl)
>>> +int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock 
>>> *fl,
>>> +		const struct nlmclnt_operations *nlmclnt_ops, void *data)
>>
>> Might it be cleaner to put the nlmclnt_operations into the nlm_host?
>> Could pass a pointer to it in the nlmclnt_initdata when creating the
>> nfs_server.
>
> Maybe, if 5 args here is too much.  But passing nlmclnt_operations 
> into
> nlm_init in nfs_start_lockd() would mean the function pointers in
> nlmclnt_operations would need to be exposed in client.c.  While this 
> has
> more args, I think it is a bit neater from the NFS side.

Jeff, I looked at this again this morning, and I can't find a cleaner 
way to
do this by putting the ops on the host.  It will mean plumbing the ops 
into
nfs_start_lockd.  The other reason is that the ops are specific to the 
proc
being passed in, so for LOCK we might use different operations.  I'm 
open to
suggestions, but I am going to send another version without this change.

Ben

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/6] lockd: Introduce nlmclnt_operations
  2017-04-01 14:05     ` Jeff Layton
  (?)
@ 2017-04-03  9:40     ` Benjamin Coddington
  2017-04-06 11:19       ` Benjamin Coddington
  -1 siblings, 1 reply; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-03  9:40 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Trond Myklebust, Anna Schumaker, bfields, Miklos Szeredi,
	Alexander Viro, linux-nfs, linux-fsdevel


On 1 Apr 2017, at 10:05, Jeff Layton wrote:

> On Fri, 2017-03-31 at 23:16 -0400, Benjamin Coddington wrote:
>> NFS would enjoy the ability to modify the behavior of the NLM 
>> client's
>> unlock RPC task in order to delay the transmission of the unlock 
>> until IO
>> that was submitted under that lock has completed.  This ability can 
>> ensure
>> that the NLM client will always complete the transmission of an 
>> unlock even
>> if the waiting caller has been interrupted with fatal signal.
>>
>> A struct nlmclnt_operations and callback data pointer can be passed 
>> to
>> nlmclnt_proc(). The struct nlmclnt_operations defines three callback
>> operations that will be used in a following patch:
>>
>> nlmclnt_alloc_call - used to call back after a successful allocation 
>> of
>> 	a struct nlm_rqst in nlmclnt_proc().
>>
>> nlmclnt_unlock_prepare - used to call back during NLM unlock's
>> 	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
>> 	until this callback returns false.
>>
>> nlmclnt_release_call - used to call back when the NLM client's struct
>> 	nlm_rqst is freed.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>  fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
>>  fs/nfs/nfs3proc.c           |  2 +-
>>  fs/nfs/proc.c               |  2 +-
>>  include/linux/lockd/bind.h  | 11 +++++++++--
>>  include/linux/lockd/lockd.h |  2 ++
>>  5 files changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
>> index 112952037933..d2744f4960ed 100644
>> --- a/fs/lockd/clntproc.c
>> +++ b/fs/lockd/clntproc.c
>> @@ -150,9 +150,12 @@ static void nlmclnt_release_lockargs(struct 
>> nlm_rqst *req)
>>   * @host: address of a valid nlm_host context representing the NLM 
>> server
>>   * @cmd: fcntl-style file lock operation to perform
>>   * @fl: address of arguments for the lock operation
>> + * @nlmclnt_ops: operations to call back out of nlm
>> + * @data: address of data to be sent along with callback
>>   *
>>   */
>> -int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock 
>> *fl)
>> +int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock 
>> *fl,
>> +		const struct nlmclnt_operations *nlmclnt_ops, void *data)
>
> Might it be cleaner to put the nlmclnt_operations into the nlm_host?
> Could pass a pointer to it in the nlmclnt_initdata when creating the
> nfs_server.

Maybe, if 5 args here is too much.  But passing nlmclnt_operations into
nlm_init in nfs_start_lockd() would mean the function pointers in
nlmclnt_operations would need to be exposed in client.c.  While this has
more args, I think it is a bit neater from the NFS side.

Ben

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/6] lockd: Introduce nlmclnt_operations
  2017-04-01  3:16 ` [PATCH 5/6] lockd: Introduce nlmclnt_operations Benjamin Coddington
@ 2017-04-01 14:05     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-01 14:05 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Fri, 2017-03-31 at 23:16 -0400, Benjamin Coddington wrote:
> NFS would enjoy the ability to modify the behavior of the NLM client's
> unlock RPC task in order to delay the transmission of the unlock until IO
> that was submitted under that lock has completed.  This ability can ensure
> that the NLM client will always complete the transmission of an unlock even
> if the waiting caller has been interrupted with fatal signal.
> 
> A struct nlmclnt_operations and callback data pointer can be passed to
> nlmclnt_proc(). The struct nlmclnt_operations defines three callback
> operations that will be used in a following patch:
> 
> nlmclnt_alloc_call - used to call back after a successful allocation of
> 	a struct nlm_rqst in nlmclnt_proc().
> 
> nlmclnt_unlock_prepare - used to call back during NLM unlock's
> 	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
> 	until this callback returns false.
> 
> nlmclnt_release_call - used to call back when the NLM client's struct
> 	nlm_rqst is freed.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
>  fs/nfs/nfs3proc.c           |  2 +-
>  fs/nfs/proc.c               |  2 +-
>  include/linux/lockd/bind.h  | 11 +++++++++--
>  include/linux/lockd/lockd.h |  2 ++
>  5 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 112952037933..d2744f4960ed 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -150,9 +150,12 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
>   * @host: address of a valid nlm_host context representing the NLM server
>   * @cmd: fcntl-style file lock operation to perform
>   * @fl: address of arguments for the lock operation
> + * @nlmclnt_ops: operations to call back out of nlm
> + * @data: address of data to be sent along with callback
>   *
>   */
> -int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
> +int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
> +		const struct nlmclnt_operations *nlmclnt_ops, void *data)

Might it be cleaner to put the nlmclnt_operations into the nlm_host?
Could pass a pointer to it in the nlmclnt_initdata when creating the
nfs_server.

>  { You would probably need some way to indicate the NLM version you
> want to use, but 
>  	struct nlm_rqst		*call;
>  	int			status;
> @@ -161,6 +164,9 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	if (call == NULL)
>  		return -ENOMEM;
>  
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
> +		nlmclnt_ops->nlmclnt_alloc_call(data);
> +
>  	nlmclnt_locks_init_private(fl, host);
>  	if (!fl->fl_u.nfs_fl.owner) {
>  		/* lockowner allocation has failed */
> @@ -169,6 +175,8 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	}
>  	/* Set up the argument struct */
>  	nlmclnt_setlockargs(call, fl);
> +	call->nlmclnt_ops = nlmclnt_ops;
> +	call->callback_data = data;
>  
>  	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
>  		if (fl->fl_type != F_UNLCK) {
> @@ -216,6 +224,8 @@ void nlmclnt_release_call(struct nlm_rqst *call)
>  {
>  	if (!atomic_dec_and_test(&call->a_count))
>  		return;
> +	if (call->nlmclnt_ops && call->nlmclnt_ops->nlmclnt_release_call)
> +		call->nlmclnt_ops->nlmclnt_release_call(call->callback_data);
>  	nlmclnt_release_host(call->a_host);
>  	nlmclnt_release_lockargs(call);
>  	kfree(call);
> @@ -687,6 +697,19 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
>  	return status;
>  }
>  
> +static void nlmclnt_unlock_prepare(struct rpc_task *task, void *data)
> +{
> +	struct nlm_rqst	*req = data;
> +	const struct nlmclnt_operations *nlmclnt_ops = req->nlmclnt_ops;
> +	bool defer_call = false;
> +
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_unlock_prepare)
> +		defer_call = nlmclnt_ops->nlmclnt_unlock_prepare(task, req->callback_data);
> +
> +	if (!defer_call)
> +		rpc_call_start(task);
> +}
> +
>  static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  {
>  	struct nlm_rqst	*req = data;
> @@ -720,6 +743,7 @@ static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  }
>  
>  static const struct rpc_call_ops nlmclnt_unlock_ops = {
> +	.rpc_call_prepare = nlmclnt_unlock_prepare,
>  	.rpc_call_done = nlmclnt_unlock_callback,
>  	.rpc_release = nlmclnt_rpc_release,
>  };
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index dc925b531f32..086623ab07b1 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -870,7 +870,7 @@ nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
>  }
>  
>  static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
> diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
> index b7bca8303989..fb8cbdf0a980 100644
> --- a/fs/nfs/proc.c
> +++ b/fs/nfs/proc.c
> @@ -638,7 +638,7 @@ nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
>  }
>  
>  /* Helper functions for NFS lock bounds checking */
> diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
> index 140edab64446..00c3c2a62bb5 100644
> --- a/include/linux/lockd/bind.h
> +++ b/include/linux/lockd/bind.h
> @@ -18,6 +18,7 @@
>  
>  /* Dummy declarations */
>  struct svc_rqst;
> +struct rpc_task;
>  
>  /*
>   * This is the set of functions for lockd->nfsd communication
> @@ -52,8 +53,14 @@ struct nlmclnt_initdata {
>  extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
>  extern void	nlmclnt_done(struct nlm_host *host);
>  
> -extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
> -					struct file_lock *fl);
> +struct nlmclnt_operations {
> +	void (*nlmclnt_alloc_call)(void *);
> +	bool (*nlmclnt_unlock_prepare)(struct rpc_task*, void *);
> +	void (*nlmclnt_release_call)(void *);
> +};
> +
> +extern int	nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
> +	const struct nlmclnt_operations *nlmclnt_ops, void *data);
>  extern int	lockd_up(struct net *net);
>  extern void	lockd_down(struct net *net);
>  
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index c15373894a42..0c6f777dde53 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -142,6 +142,8 @@ struct nlm_rqst {
>  	struct nlm_block *	a_block;
>  	unsigned int		a_retries;	/* Retry count */
>  	u8			a_owner[NLMCLNT_OHSIZE];
> +	const struct nlmclnt_operations * nlmclnt_ops;
> +	void * callback_data;
>  };
>  
>  /*

-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/6] lockd: Introduce nlmclnt_operations
@ 2017-04-01 14:05     ` Jeff Layton
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2017-04-01 14:05 UTC (permalink / raw)
  To: Benjamin Coddington, Trond Myklebust, Anna Schumaker, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

On Fri, 2017-03-31 at 23:16 -0400, Benjamin Coddington wrote:
> NFS would enjoy the ability to modify the behavior of the NLM client's
> unlock RPC task in order to delay the transmission of the unlock until IO
> that was submitted under that lock has completed.  This ability can ensure
> that the NLM client will always complete the transmission of an unlock even
> if the waiting caller has been interrupted with fatal signal.
> 
> A struct nlmclnt_operations and callback data pointer can be passed to
> nlmclnt_proc(). The struct nlmclnt_operations defines three callback
> operations that will be used in a following patch:
> 
> nlmclnt_alloc_call - used to call back after a successful allocation of
> 	a struct nlm_rqst in nlmclnt_proc().
> 
> nlmclnt_unlock_prepare - used to call back during NLM unlock's
> 	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
> 	until this callback returns false.
> 
> nlmclnt_release_call - used to call back when the NLM client's struct
> 	nlm_rqst is freed.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
>  fs/nfs/nfs3proc.c           |  2 +-
>  fs/nfs/proc.c               |  2 +-
>  include/linux/lockd/bind.h  | 11 +++++++++--
>  include/linux/lockd/lockd.h |  2 ++
>  5 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 112952037933..d2744f4960ed 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -150,9 +150,12 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
>   * @host: address of a valid nlm_host context representing the NLM server
>   * @cmd: fcntl-style file lock operation to perform
>   * @fl: address of arguments for the lock operation
> + * @nlmclnt_ops: operations to call back out of nlm
> + * @data: address of data to be sent along with callback
>   *
>   */
> -int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
> +int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
> +		const struct nlmclnt_operations *nlmclnt_ops, void *data)

Might it be cleaner to put the nlmclnt_operations into the nlm_host?
Could pass a pointer to it in the nlmclnt_initdata when creating the
nfs_server.

>  { You would probably need some way to indicate the NLM version you
> want to use, but 
>  	struct nlm_rqst		*call;
>  	int			status;
> @@ -161,6 +164,9 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	if (call == NULL)
>  		return -ENOMEM;
>  
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
> +		nlmclnt_ops->nlmclnt_alloc_call(data);
> +
>  	nlmclnt_locks_init_private(fl, host);
>  	if (!fl->fl_u.nfs_fl.owner) {
>  		/* lockowner allocation has failed */
> @@ -169,6 +175,8 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
>  	}
>  	/* Set up the argument struct */
>  	nlmclnt_setlockargs(call, fl);
> +	call->nlmclnt_ops = nlmclnt_ops;
> +	call->callback_data = data;
>  
>  	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
>  		if (fl->fl_type != F_UNLCK) {
> @@ -216,6 +224,8 @@ void nlmclnt_release_call(struct nlm_rqst *call)
>  {
>  	if (!atomic_dec_and_test(&call->a_count))
>  		return;
> +	if (call->nlmclnt_ops && call->nlmclnt_ops->nlmclnt_release_call)
> +		call->nlmclnt_ops->nlmclnt_release_call(call->callback_data);
>  	nlmclnt_release_host(call->a_host);
>  	nlmclnt_release_lockargs(call);
>  	kfree(call);
> @@ -687,6 +697,19 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
>  	return status;
>  }
>  
> +static void nlmclnt_unlock_prepare(struct rpc_task *task, void *data)
> +{
> +	struct nlm_rqst	*req = data;
> +	const struct nlmclnt_operations *nlmclnt_ops = req->nlmclnt_ops;
> +	bool defer_call = false;
> +
> +	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_unlock_prepare)
> +		defer_call = nlmclnt_ops->nlmclnt_unlock_prepare(task, req->callback_data);
> +
> +	if (!defer_call)
> +		rpc_call_start(task);
> +}
> +
>  static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  {
>  	struct nlm_rqst	*req = data;
> @@ -720,6 +743,7 @@ static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
>  }
>  
>  static const struct rpc_call_ops nlmclnt_unlock_ops = {
> +	.rpc_call_prepare = nlmclnt_unlock_prepare,
>  	.rpc_call_done = nlmclnt_unlock_callback,
>  	.rpc_release = nlmclnt_rpc_release,
>  };
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index dc925b531f32..086623ab07b1 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -870,7 +870,7 @@ nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
>  }
>  
>  static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
> diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
> index b7bca8303989..fb8cbdf0a980 100644
> --- a/fs/nfs/proc.c
> +++ b/fs/nfs/proc.c
> @@ -638,7 +638,7 @@ nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
>  {
>  	struct inode *inode = file_inode(filp);
>  
> -	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
> +	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
>  }
>  
>  /* Helper functions for NFS lock bounds checking */
> diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
> index 140edab64446..00c3c2a62bb5 100644
> --- a/include/linux/lockd/bind.h
> +++ b/include/linux/lockd/bind.h
> @@ -18,6 +18,7 @@
>  
>  /* Dummy declarations */
>  struct svc_rqst;
> +struct rpc_task;
>  
>  /*
>   * This is the set of functions for lockd->nfsd communication
> @@ -52,8 +53,14 @@ struct nlmclnt_initdata {
>  extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
>  extern void	nlmclnt_done(struct nlm_host *host);
>  
> -extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
> -					struct file_lock *fl);
> +struct nlmclnt_operations {
> +	void (*nlmclnt_alloc_call)(void *);
> +	bool (*nlmclnt_unlock_prepare)(struct rpc_task*, void *);
> +	void (*nlmclnt_release_call)(void *);
> +};
> +
> +extern int	nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
> +	const struct nlmclnt_operations *nlmclnt_ops, void *data);
>  extern int	lockd_up(struct net *net);
>  extern void	lockd_down(struct net *net);
>  
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index c15373894a42..0c6f777dde53 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -142,6 +142,8 @@ struct nlm_rqst {
>  	struct nlm_block *	a_block;
>  	unsigned int		a_retries;	/* Retry count */
>  	u8			a_owner[NLMCLNT_OHSIZE];
> +	const struct nlmclnt_operations * nlmclnt_ops;
> +	void * callback_data;
>  };
>  
>  /*

-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 5/6] lockd: Introduce nlmclnt_operations
  2017-04-01  3:15 [PATCH v4 0/6] Skipped unlocks Benjamin Coddington
@ 2017-04-01  3:16 ` Benjamin Coddington
  2017-04-01 14:05     ` Jeff Layton
  0 siblings, 1 reply; 26+ messages in thread
From: Benjamin Coddington @ 2017-04-01  3:16 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, Jeff Layton, bfields,
	Miklos Szeredi, Alexander Viro
  Cc: linux-nfs, linux-fsdevel

NFS would enjoy the ability to modify the behavior of the NLM client's
unlock RPC task in order to delay the transmission of the unlock until IO
that was submitted under that lock has completed.  This ability can ensure
that the NLM client will always complete the transmission of an unlock even
if the waiting caller has been interrupted with fatal signal.

A struct nlmclnt_operations and callback data pointer can be passed to
nlmclnt_proc(). The struct nlmclnt_operations defines three callback
operations that will be used in a following patch:

nlmclnt_alloc_call - used to call back after a successful allocation of
	a struct nlm_rqst in nlmclnt_proc().

nlmclnt_unlock_prepare - used to call back during NLM unlock's
	rpc_call_prepare.  The NLM client defers calling rpc_call_start()
	until this callback returns false.

nlmclnt_release_call - used to call back when the NLM client's struct
	nlm_rqst is freed.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/lockd/clntproc.c         | 26 +++++++++++++++++++++++++-
 fs/nfs/nfs3proc.c           |  2 +-
 fs/nfs/proc.c               |  2 +-
 include/linux/lockd/bind.h  | 11 +++++++++--
 include/linux/lockd/lockd.h |  2 ++
 5 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 112952037933..d2744f4960ed 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -150,9 +150,12 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
  * @host: address of a valid nlm_host context representing the NLM server
  * @cmd: fcntl-style file lock operation to perform
  * @fl: address of arguments for the lock operation
+ * @nlmclnt_ops: operations to call back out of nlm
+ * @data: address of data to be sent along with callback
  *
  */
-int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
+int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
+		const struct nlmclnt_operations *nlmclnt_ops, void *data)
 {
 	struct nlm_rqst		*call;
 	int			status;
@@ -161,6 +164,9 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
 	if (call == NULL)
 		return -ENOMEM;
 
+	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
+		nlmclnt_ops->nlmclnt_alloc_call(data);
+
 	nlmclnt_locks_init_private(fl, host);
 	if (!fl->fl_u.nfs_fl.owner) {
 		/* lockowner allocation has failed */
@@ -169,6 +175,8 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
 	}
 	/* Set up the argument struct */
 	nlmclnt_setlockargs(call, fl);
+	call->nlmclnt_ops = nlmclnt_ops;
+	call->callback_data = data;
 
 	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
 		if (fl->fl_type != F_UNLCK) {
@@ -216,6 +224,8 @@ void nlmclnt_release_call(struct nlm_rqst *call)
 {
 	if (!atomic_dec_and_test(&call->a_count))
 		return;
+	if (call->nlmclnt_ops && call->nlmclnt_ops->nlmclnt_release_call)
+		call->nlmclnt_ops->nlmclnt_release_call(call->callback_data);
 	nlmclnt_release_host(call->a_host);
 	nlmclnt_release_lockargs(call);
 	kfree(call);
@@ -687,6 +697,19 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
 	return status;
 }
 
+static void nlmclnt_unlock_prepare(struct rpc_task *task, void *data)
+{
+	struct nlm_rqst	*req = data;
+	const struct nlmclnt_operations *nlmclnt_ops = req->nlmclnt_ops;
+	bool defer_call = false;
+
+	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_unlock_prepare)
+		defer_call = nlmclnt_ops->nlmclnt_unlock_prepare(task, req->callback_data);
+
+	if (!defer_call)
+		rpc_call_start(task);
+}
+
 static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
 {
 	struct nlm_rqst	*req = data;
@@ -720,6 +743,7 @@ static void nlmclnt_unlock_callback(struct rpc_task *task, void *data)
 }
 
 static const struct rpc_call_ops nlmclnt_unlock_ops = {
+	.rpc_call_prepare = nlmclnt_unlock_prepare,
 	.rpc_call_done = nlmclnt_unlock_callback,
 	.rpc_release = nlmclnt_rpc_release,
 };
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index dc925b531f32..086623ab07b1 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -870,7 +870,7 @@ nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = file_inode(filp);
 
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
 }
 
 static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index b7bca8303989..fb8cbdf0a980 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -638,7 +638,7 @@ nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = file_inode(filp);
 
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL);
 }
 
 /* Helper functions for NFS lock bounds checking */
diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
index 140edab64446..00c3c2a62bb5 100644
--- a/include/linux/lockd/bind.h
+++ b/include/linux/lockd/bind.h
@@ -18,6 +18,7 @@
 
 /* Dummy declarations */
 struct svc_rqst;
+struct rpc_task;
 
 /*
  * This is the set of functions for lockd->nfsd communication
@@ -52,8 +53,14 @@ struct nlmclnt_initdata {
 extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
 extern void	nlmclnt_done(struct nlm_host *host);
 
-extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
-					struct file_lock *fl);
+struct nlmclnt_operations {
+	void (*nlmclnt_alloc_call)(void *);
+	bool (*nlmclnt_unlock_prepare)(struct rpc_task*, void *);
+	void (*nlmclnt_release_call)(void *);
+};
+
+extern int	nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,
+	const struct nlmclnt_operations *nlmclnt_ops, void *data);
 extern int	lockd_up(struct net *net);
 extern void	lockd_down(struct net *net);
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index c15373894a42..0c6f777dde53 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -142,6 +142,8 @@ struct nlm_rqst {
 	struct nlm_block *	a_block;
 	unsigned int		a_retries;	/* Retry count */
 	u8			a_owner[NLMCLNT_OHSIZE];
+	const struct nlmclnt_operations * nlmclnt_ops;
+	void * callback_data;
 };
 
 /*
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2017-04-21 12:25 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-06 11:23 [PATCH v5 0/6] Skipped unlocks Benjamin Coddington
2017-04-06 11:23 ` [PATCH 1/6] NFS4: remove a redundant lock range check Benjamin Coddington
2017-04-06 11:23 ` [PATCH 2/6] NFS: Move the flock open mode check into nfs_flock() Benjamin Coddington
2017-04-06 11:23 ` [PATCH 3/6] locks: Set FL_CLOSE when removing flock locks on close() Benjamin Coddington
2017-04-06 18:17   ` Jeff Layton
2017-04-06 18:17     ` Jeff Layton
2017-04-08  5:34   ` kbuild test robot
2017-04-06 11:23 ` [PATCH 4/6] NFS: Add an iocounter wait function for async RPC tasks Benjamin Coddington
2017-04-07 10:44   ` Jeff Layton
2017-04-07 10:44     ` Jeff Layton
2017-04-07 11:26     ` Benjamin Coddington
2017-04-06 11:23 ` [PATCH 5/6] lockd: Introduce nlmclnt_operations Benjamin Coddington
2017-04-07 12:10   ` Jeff Layton
2017-04-07 12:10     ` Jeff Layton
2017-04-06 11:23 ` [PATCH 6/6] NFS: Always wait for I/O completion before unlock Benjamin Coddington
2017-04-07 12:22   ` Jeff Layton
2017-04-07 12:22     ` Jeff Layton
2017-04-07 13:34     ` Benjamin Coddington
  -- strict thread matches above, loose matches on Subject: below --
2017-04-11 16:50 [PATCH v6 0/6] Skipped unlocks Benjamin Coddington
2017-04-11 16:50 ` [PATCH 5/6] lockd: Introduce nlmclnt_operations Benjamin Coddington
2017-04-21 12:25   ` Jeff Layton
2017-04-21 12:25     ` Jeff Layton
2017-04-01  3:15 [PATCH v4 0/6] Skipped unlocks Benjamin Coddington
2017-04-01  3:16 ` [PATCH 5/6] lockd: Introduce nlmclnt_operations Benjamin Coddington
2017-04-01 14:05   ` Jeff Layton
2017-04-01 14:05     ` Jeff Layton
2017-04-03  9:40     ` Benjamin Coddington
2017-04-06 11:19       ` Benjamin Coddington

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.