Re: [PATCH v4 08/12] NFS handle COPY reply CB_OFFLOAD call race

[Anna Schumaker <schumaker.anna@gmail.com>]

Hi Olga,

On 09/28/2017 01:28 PM, Olga Kornievskaia wrote:
> It's possible that server replies back with CB_OFFLOAD call and
> COPY reply at the same time such that client will process
> CB_OFFLOAD before reply to COPY. For that keep a list of pending
> callback stateids received and them before waiting on completion
                    ^^^^^^^^^^^^^^^^^^^^^^^^
What are you doing with the stateids?

> check the pending list.
> 
> Cleanup any pending copies on the client shutdown.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/callback_proc.c    | 17 ++++++++++++++---
>  fs/nfs/nfs42proc.c        | 24 +++++++++++++++++++++---
>  fs/nfs/nfs4client.c       | 15 +++++++++++++++
>  include/linux/nfs_fs_sb.h |  1 +
>  4 files changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index d3e7b61..d546c9f 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -676,11 +676,12 @@ __be32 nfs4_callback_offload(void *data, void *dummy,
>  	struct cb_offloadargs *args = data;
>  	struct nfs_server *server;
>  	struct nfs4_copy_state *copy;
> +	bool found = false;
>  
> +	spin_lock(&cps->clp->cl_lock);
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(server, &cps->clp->cl_superblocks,
>  				client_link) {
> -		spin_lock(&server->nfs_client->cl_lock);
>  		list_for_each_entry(copy, &server->ss_copies, copies) {
>  			if (memcmp(args->coa_stateid.other,
>  					copy->stateid.other,
> @@ -688,13 +689,23 @@ __be32 nfs4_callback_offload(void *data, void *dummy,
>  				continue;
>  			nfs4_copy_cb_args(copy, args);
>  			complete(&copy->completion);
> -			spin_unlock(&server->nfs_client->cl_lock);
> +			found = true;
>  			goto out;
>  		}
> -		spin_unlock(&server->nfs_client->cl_lock);
>  	}
>  out:
>  	rcu_read_unlock();
> +	if (!found) {
> +		copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
> +		if (!copy) {
> +			spin_unlock(&cps->clp->cl_lock);
> +			return -ENOMEM;
> +		}
> +		memcpy(&copy->stateid, &args->coa_stateid, NFS4_STATEID_SIZE);
> +		nfs4_copy_cb_args(copy, args);
> +		list_add_tail(&copy->copies, &cps->clp->pending_cb_stateids);
> +	}
> +	spin_unlock(&cps->clp->cl_lock);
>  
>  	return 0;
>  }
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index c8ca353..d0f454e 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -137,15 +137,32 @@ static int handle_async_copy(struct nfs42_copy_res *res,
>  			     uint64_t *ret_count)
>  {
>  	struct nfs4_copy_state *copy;
> -	int status;
> +	int status = NFS4_OK;

Might as well just move this bit to the previous patch :)

Anna

> +	bool found_pending = false;
> +
> +	spin_lock(&server->nfs_client->cl_lock);
> +	list_for_each_entry(copy, &server->nfs_client->pending_cb_stateids,
> +				copies) {
> +		if (memcmp(&res->write_res.stateid, &copy->stateid,
> +				NFS4_STATEID_SIZE))
> +			continue;
> +		found_pending = true;
> +		list_del(&copy->copies);
> +		break;
> +	}
> +	if (found_pending) {
> +		spin_unlock(&server->nfs_client->cl_lock);
> +		goto out;
> +	}
>  
>  	copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
> -	if (!copy)
> +	if (!copy) {
> +		spin_unlock(&server->nfs_client->cl_lock);
>  		return -ENOMEM;
> +	}
>  	memcpy(&copy->stateid, &res->write_res.stateid, NFS4_STATEID_SIZE);
>  	init_completion(&copy->completion);
>  
> -	spin_lock(&server->nfs_client->cl_lock);
>  	list_add_tail(&copy->copies, &server->ss_copies);
>  	spin_unlock(&server->nfs_client->cl_lock);
>  
> @@ -153,6 +170,7 @@ static int handle_async_copy(struct nfs42_copy_res *res,
>  	spin_lock(&server->nfs_client->cl_lock);
>  	list_del_init(&copy->copies);
>  	spin_unlock(&server->nfs_client->cl_lock);
> +out:
>  	*ret_count = copy->count;
>  	memcpy(&res->write_res.verifier, &copy->verf, sizeof(copy->verf));
>  	if (copy->count <= 0)
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index e9bea90..46d4649 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -156,9 +156,23 @@ struct rpc_clnt *
>  	}
>  }
>  
> +static void
> +nfs4_cleanup_callback(struct nfs_client *clp)
> +{
> +	struct nfs4_copy_state *cp_state;
> +
> +	while (!list_empty(&clp->pending_cb_stateids)) {
> +		cp_state = list_entry(clp->pending_cb_stateids.next,
> +					struct nfs4_copy_state, copies);
> +		list_del(&cp_state->copies);
> +		kfree(cp_state);
> +	}
> +}
> +
>  void nfs41_shutdown_client(struct nfs_client *clp)
>  {
>  	if (nfs4_has_session(clp)) {
> +		nfs4_cleanup_callback(clp);
>  		nfs4_shutdown_ds_clients(clp);
>  		nfs4_destroy_session(clp->cl_session);
>  		nfs4_destroy_clientid(clp);
> @@ -202,6 +216,7 @@ struct nfs_client *nfs4_alloc_client(const struct nfs_client_initdata *cl_init)
>  #if IS_ENABLED(CONFIG_NFS_V4_1)
>  	init_waitqueue_head(&clp->cl_lock_waitq);
>  #endif
> +	INIT_LIST_HEAD(&clp->pending_cb_stateids);
>  	return clp;
>  
>  error:
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index 511eefb..72f159e 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -119,6 +119,7 @@ struct nfs_client {
>  #endif
>  
>  	struct net		*cl_net;
> +	struct list_head	pending_cb_stateids;
>  };
>  
>  /*
>