[5/6] lockd: Introduce nlmclnt_operations
diff mbox

Message ID c2c20de92b177386ad627726b6b2838569c2b28d.1491015671.git.bcodding@redhat.com
State New
Headers show

Commit Message

Benjamin Coddington April 1, 2017, 3:16 a.m. UTC
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(-)

Comments

Jeff Layton April 1, 2017, 2:05 p.m. UTC | #1
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;
>  };
>  
>  /*
Benjamin Coddington April 3, 2017, 9:40 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Coddington April 6, 2017, 11:19 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;
 };
 
 /*