Re: [PATCH 5/6] lockd: Introduce nlmclnt_operations

[Jeff Layton <jlayton@redhat.com>]

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>