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

[Olga Kornievskaia <aglo@umich.edu>]

On Thu, Sep 28, 2017 at 3:50 PM, Anna Schumaker
<schumaker.anna@gmail.com> wrote:
> 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?

Is fixing "them" to "then" address the question?

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

Got it.

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