Skipped unlocks

flock64_to_posix_lock() is already doing this check

Signed-off-by Benjamin Coddington <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/nfs/nfs4proc.c | 3 ---
 1 file changed, 3 deletions(-)

fs/nfs/nfs4proc.c +0 -0

index 0a0eaecf9676..9388899e4050 100644

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

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

---
 fs/nfs/file.c     | 18 ++++++++++++++++--
 fs/nfs/nfs4proc.c | 14 --------------
 2 files changed, 16 insertions(+), 16 deletions(-)

fs/nfs/file.c +16 -2

index 26dbe8b0c10d..a490f45df4db 100644
@@ -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);

fs/nfs/nfs4proc.c +0 -7

index 9388899e4050..91f88bfbbe79 100644
@@ -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))

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.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/fuse/file.c     | 2 +-
 fs/locks.c         | 2 +-
 include/linux/fs.h | 2 ++
 3 files changed, 4 insertions(+), 2 deletions(-)

fs/fuse/file.c +1 -1

index 2401c5dabb2a..79e1a3a048a8 100644
@@ -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);

fs/locks.c +1 -1

index 26811321d39b..af2031a1fcff 100644
@@ -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,
 	};

include/linux/fs.h +0 -0

index 2ba074328894..e964ae0a9c29 100644

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

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

fs/lockd/clntproc.c +0 -0

index 112952037933..d2744f4960ed 100644

fs/lockd/clntproc.c +0 -0

 { You would probably need some way to indicate the NLM version you

want to use, but

fs/lockd/clntproc.c +21 -0

 	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,
 };

fs/nfs/nfs3proc.c +1 -1

index dc925b531f32..086623ab07b1 100644
@@ -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)

fs/nfs/proc.c +1 -1

index b7bca8303989..fb8cbdf0a980 100644
@@ -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 */

include/linux/lockd/bind.h +9 -2

index 140edab64446..00c3c2a62bb5 100644
@@ -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);
 

include/linux/lockd/lockd.h +2 -0

index c15373894a42..0c6f777dde53 100644
@@ -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;
 };
 
 /*

Quoted 44 lines · Benjamin Coddington

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

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

fs/lockd/clntproc.c +0 -0

index 112952037933..d2744f4960ed 100644
@@ -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

fs/lockd/clntproc.c static void nlmclnt_release_lockargs(struct nlm_rqst *req) · line 158

Quoted 4 lines

*fl)
+int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,

fs/lockd/clntproc.c +1 -0

+		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.

Hi Benjamin,

[auto build test ERROR on nfs/linux-next]
[also build test ERROR on v4.11-rc4 next-20170331]
[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/20170402-195846
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: cris-etrax-100lx_v2_defconfig (attached as .config)
compiler: cris-linux-gcc (GCC) 6.2.0

reproduce:
wget
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree make.cross ARCH=cris

All errors (new ones prefixed by >>):

fs/nfs/pagelist.c: In function 'nfs_async_iocounter_wait':

Quoted 1 line

Quoted 1 line

fs/nfs/pagelist.c:126:34: error: 'struct nfs_server' has no member named 'uoc_rpcwaitq'

      rpc_sleep_on(&NFS_SERVER(inode)->uoc_rpcwaitq, task, NULL);
                                     ^~
   fs/nfs/pagelist.c:131:45: error: 'struct nfs_server' has no member 
named 'uoc_rpcwaitq'
      rpc_wake_up_queued_task(&NFS_SERVER(inode)->uoc_rpcwaitq, task);
                                                ^~
   fs/nfs/pagelist.c: In function 'nfs_clear_request':
   fs/nfs/pagelist.c:425:50: error: 'struct nfs_server' has no member 
named 'uoc_rpcwaitq'
        rpc_wake_up(&NFS_SERVER(d_inode(ctx->dentry))->uoc_rpcwaitq);
                                                     ^~

vim +126 fs/nfs/pagelist.c

   120	{
   121		struct nfs_lock_context *l_ctx = data;
   122		struct inode *inode = d_inode(l_ctx->open_context->dentry);
   123		bool ret = false;
   124	
   125		if (atomic_read(&l_ctx->io_count) > 0) {

Quoted 1 line

126 rpc_sleep_on(&NFS_SERVER(inode)->uoc_rpcwaitq, task, NULL);

   127			ret = true;
   128		}
   129	

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

Quoted 4 lines · Benjamin Coddington

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.

Might be nice to add some verbiage here about why fuse needs to be changed, just for posterity. I recall the discussion a while back so I think the change itself looks fine.

Quoted 45 lines · Jeff Layton

Quoted 44 lines · Benjamin Coddington

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

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

fs/lockd/clntproc.c +0 -0

index 112952037933..d2744f4960ed 100644
@@ -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

Quoted 8 lines

fs/lockd/clntproc.c static void nlmclnt_release_lockargs(struct nlm_rqst *req) · line 158

Quoted 4 lines

*fl)
+int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl,

fs/lockd/clntproc.c +1 -0

+		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.