On Thu, Sep 28, 2017 at 3:12 PM, J. Bruce Fields <bfields@xxxxxxxxxx> wrote:
> On Thu, Sep 28, 2017 at 01:29:42PM -0400, Olga Kornievskaia wrote:
>> Generate a new stateid to be used for reply to the asynchronous
>> COPY (this would also be used later by COPY_NOTIFY as well).
>> Associate the stateid with the parent OPEN/LOCK/DELEG stateid
>> that can be freed during the free of the parent stateid. However,
>> right now deciding to bind the lifetime to when the vfs copy
>> is done. This way don't need to keep the nfsd_net structure for
>> the callback. The drawback is that time copy state information
>> is available for query by OFFLOAD_STATUS is slightly less.
>
> I don't think we're under any obligation to handle OFFLOAD_STATUS after
> the copy is done, are we?
It depends on how you define “copy is done”. I’d say copy is done once the
server sent the CB_OFFLOAD. But I’m choosing to clear the state once the
vfs_copy_file_range() is done. Then the callback is queued up (what if
there is wait until the callback can be processed) and there is a period
where if the client were to send a OFFLOAD_STATUS it should get the
bytes representing the full copy but instead the client will get an error back.
I was worried about what if some implementation depend on querying for
the status and stopping to poll only when they received that all bytes were
copied and they might not get that with how the server is coded right now?
Thus I thought I’d bring it up to decide if the freeing should be moved into
the callback process (but then more stuff needed to be copied)….
> There's a possibility that OFFLOAD_STATUS might arrive just as the copy
> finishes, and the client might even received the failed response to
> OFFLOAD_STATUS before it receives the final CB_OFFLOAD. I think it's up
> to the client to handle that situation.
>
> --b.
>
>>
>> Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
>> ---
>> fs/nfsd/netns.h | 8 +++++++
>> fs/nfsd/nfs4proc.c | 31 ++++++++++++++++++-------
>> fs/nfsd/nfs4state.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> fs/nfsd/nfsctl.c | 1 +
>> fs/nfsd/state.h | 14 ++++++++++++
>> fs/nfsd/xdr4.h | 2 ++
>> 6 files changed, 114 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>> index 3714231..2c88a95 100644
>> --- a/fs/nfsd/netns.h
>> +++ b/fs/nfsd/netns.h
>> @@ -119,6 +119,14 @@ struct nfsd_net {
>> u32 clverifier_counter;
>>
>> struct svc_serv *nfsd_serv;
>> +
>> + /*
>> + * clientid and stateid data for construction of net unique COPY
>> + * stateids.
>> + */
>> + u32 s2s_cp_cl_id;
>> + struct idr s2s_cp_stateids;
>> + spinlock_t s2s_cp_lock;
>> };
>>
>> /* Simple check to find out if a given net was properly initialized */
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 044af6b..3cddebb 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -1046,7 +1046,8 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>> static __be32
>> nfsd4_verify_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> stateid_t *src_stateid, struct file **src,
>> - stateid_t *dst_stateid, struct file **dst)
>> + stateid_t *dst_stateid, struct file **dst,
>> + struct nfs4_stid **stid)
>> {
>> __be32 status;
>>
>> @@ -1060,7 +1061,7 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>>
>> status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>> dst_stateid, WR_STATE, dst, NULL,
>> - NULL);
>> + stid);
>> if (status) {
>> dprintk("NFSD: %s: couldn't process dst stateid!\n", __func__);
>> goto out_put_src;
>> @@ -1091,7 +1092,7 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>> __be32 status;
>>
>> status = nfsd4_verify_copy(rqstp, cstate, &clone->cl_src_stateid, &src,
>> - &clone->cl_dst_stateid, &dst);
>> + &clone->cl_dst_stateid, &dst, NULL);
>> if (status)
>> goto out;
>>
>> @@ -1123,8 +1124,6 @@ static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
>>
>> static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
>> {
>> - memcpy(©->cp_res.cb_stateid, ©->cp_dst_stateid,
>> - sizeof(copy->cp_dst_stateid));
>> copy->cp_res.wr_stable_how = NFS_UNSTABLE;
>> copy->cp_consecutive = 1;
>> copy->cp_synchronous = sync;
>> @@ -1188,6 +1187,8 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
>> dst->fh_src = src->fh_src;
>> dst->fh_dst = src->fh_dst;
>> dst->net = src->net;
>> + dst->stid = src->stid;
>> + dst->cps = src->cps;
>> }
>>
>> static void nfsd4_do_async_copy(struct work_struct *work)
>> @@ -1208,6 +1209,9 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>> &nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
>> nfsd4_run_cb(&cb_copy->cp_cb);
>> out:
>> + list_del(©->cps->cp_list);
>> + nfs4_free_cp_state(copy->cps);
>> + nfs4_put_stid(copy->stid);
>> kfree(copy);
>> }
>>
>> @@ -1220,7 +1224,7 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>>
>> status = nfsd4_verify_copy(rqstp, cstate, ©->cp_src_stateid,
>> ©->fh_src, ©->cp_dst_stateid,
>> - ©->fh_dst);
>> + ©->fh_dst, ©->stid);
>> if (status)
>> goto out;
>>
>> @@ -1229,6 +1233,7 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>> sizeof(struct knfsd_fh));
>> copy->net = SVC_NET(rqstp);
>> if (!copy->cp_synchronous) {
>> + struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>> struct nfsd4_copy *async_copy;
>>
>> status = nfsd4_init_copy_res(copy, 0);
>> @@ -1237,9 +1242,19 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>> status = nfserrno(-ENOMEM);
>> goto out;
>> }
>> + copy->cps = nfs4_alloc_init_cp_state(nn, copy->stid);
>> + if (!copy->cps) {
>> + status = nfserrno(-ENOMEM);
>> + kfree(async_copy);
>> + goto out;
>> + }
>> + /* take a reference on the parent stateid so it's not
>> + * not freed by the copy compound
>> + */
>> + atomic_inc(©->stid->sc_count);
>> + memcpy(©->cp_res.cb_stateid, ©->cps->cp_stateid,
>> + sizeof(copy->cps->cp_stateid));
>> dup_copy_fields(copy, async_copy);
>> - memcpy(©->cp_res.cb_stateid, ©->cp_dst_stateid,
>> - sizeof(copy->cp_dst_stateid));
>> INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
>> queue_work(copy_wq, &async_copy->cp_work);
>> } else {
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 2c89dae..be59baf 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -658,6 +658,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
>> /* Will be incremented before return to client: */
>> atomic_set(&stid->sc_count, 1);
>> spin_lock_init(&stid->sc_lock);
>> + INIT_LIST_HEAD(&stid->sc_cp_list);
>>
>> /*
>> * It shouldn't be a problem to reuse an opaque stateid value.
>> @@ -674,6 +675,66 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
>> return NULL;
>> }
>>
>> +/*
>> + * Create a unique stateid_t to represent each COPY. Hang the copy
>> + * stateids off the OPEN/LOCK/DELEG stateid from the client open
>> + * the source file.
>> + */
>> +struct nfs4_cp_state *nfs4_alloc_init_cp_state(struct nfsd_net *nn,
>> + struct nfs4_stid *p_stid)
>> +{
>> + struct nfs4_cp_state *cps;
>> + int new_id;
>> +
>> + cps = kzalloc(sizeof(struct nfs4_cp_state), GFP_KERNEL);
>> + if (!cps)
>> + return NULL;
>> + idr_preload(GFP_KERNEL);
>> + spin_lock(&nn->s2s_cp_lock);
>> + new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, cps, 0, 0, GFP_NOWAIT);
>> + spin_unlock(&nn->s2s_cp_lock);
>> + idr_preload_end();
>> + if (new_id < 0)
>> + goto out_free;
>> + cps->cp_stateid.si_opaque.so_id = new_id;
>> + cps->cp_stateid.si_opaque.so_clid.cl_boot = nn->boot_time;
>> + cps->cp_stateid.si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
>> + cps->cp_p_stid = p_stid;
>> + INIT_LIST_HEAD(&cps->cp_list);
>> + list_add(&cps->cp_list, &p_stid->sc_cp_list);
>> +
>> + return cps;
>> +out_free:
>> + kfree(cps);
>> + return NULL;
>> +}
>> +
>> +void nfs4_free_cp_state(struct nfs4_cp_state *cps)
>> +{
>> + struct nfsd_net *nn;
>> +
>> + nn = net_generic(cps->cp_p_stid->sc_client->net, nfsd_net_id);
>> + spin_lock(&nn->s2s_cp_lock);
>> + idr_remove(&nn->s2s_cp_stateids, cps->cp_stateid.si_opaque.so_id);
>> + spin_unlock(&nn->s2s_cp_lock);
>> +
>> + kfree(cps);
>> +}
>> +
>> +static void nfs4_free_cp_statelist(struct nfs4_stid *stid)
>> +{
>> + struct nfs4_cp_state *cps;
>> +
>> + might_sleep();
>> +
>> + while (!list_empty(&stid->sc_cp_list)) {
>> + cps = list_first_entry(&stid->sc_cp_list, struct nfs4_cp_state,
>> + cp_list);
>> + list_del(&cps->cp_list);
>> + nfs4_free_cp_state(cps);
>> + }
>> +}
>> +
>> static struct nfs4_ol_stateid * nfs4_alloc_open_stateid(struct nfs4_client *clp)
>> {
>> struct nfs4_stid *stid;
>> @@ -819,6 +880,9 @@ static void block_delegations(struct knfsd_fh *fh)
>> }
>> idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
>> spin_unlock(&clp->cl_lock);
>> +
>> + nfs4_free_cp_statelist(s);
>> +
>> s->sc_free(s);
>> if (fp)
>> put_nfs4_file(fp);
>> @@ -6952,6 +7016,8 @@ static int nfs4_state_create_net(struct net *net)
>> INIT_LIST_HEAD(&nn->close_lru);
>> INIT_LIST_HEAD(&nn->del_recall_lru);
>> spin_lock_init(&nn->client_lock);
>> + spin_lock_init(&nn->s2s_cp_lock);
>> + idr_init(&nn->s2s_cp_stateids);
>>
>> spin_lock_init(&nn->blocked_locks_lock);
>> INIT_LIST_HEAD(&nn->blocked_locks_lru);
>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>> index 6493df6..232270d 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -1241,6 +1241,7 @@ static __net_init int nfsd_init_net(struct net *net)
>> nn->nfsd4_grace = 90;
>> nn->clverifier_counter = prandom_u32();
>> nn->clientid_counter = prandom_u32();
>> + nn->s2s_cp_cl_id = nn->clientid_counter++;
>> return 0;
>>
>> out_idmap_error:
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index cfd13f0..8724955 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -93,6 +93,7 @@ struct nfs4_stid {
>> #define NFS4_REVOKED_DELEG_STID 16
>> #define NFS4_CLOSED_DELEG_STID 32
>> #define NFS4_LAYOUT_STID 64
>> + struct list_head sc_cp_list;
>> unsigned char sc_type;
>> stateid_t sc_stateid;
>> spinlock_t sc_lock;
>> @@ -102,6 +103,17 @@ struct nfs4_stid {
>> };
>>
>> /*
>> + * Keep a list of stateids issued by the COPY, associate it with the
>> + * parent OPEN/LOCK/DELEG stateid. Used for lookup by
>> + * OFFLOAD_CANCEL and OFFLOAD_STATUS (as well as COPY_NOTIFY)
>> + */
>> +struct nfs4_cp_state {
>> + stateid_t cp_stateid;
>> + struct list_head cp_list; /* per parent nfs4_stid */
>> + struct nfs4_stid *cp_p_stid; /* pointer to parent */
>> +};
>> +
>> +/*
>> * Represents a delegation stateid. The nfs4_client holds references to these
>> * and they are put when it is being destroyed or when the delegation is
>> * returned by the client:
>> @@ -607,6 +619,8 @@ __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>> struct nfs4_stid **s, struct nfsd_net *nn);
>> struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *slab,
>> void (*sc_free)(struct nfs4_stid *));
>> +struct nfs4_cp_state *nfs4_alloc_init_cp_state(struct nfsd_net *nn, struct nfs4_stid *p_stid);
>> +void nfs4_free_cp_state(struct nfs4_cp_state *cps);
>> void nfs4_unhash_stid(struct nfs4_stid *s);
>> void nfs4_put_stid(struct nfs4_stid *s);
>> void nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid);
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index f7a94d3..4ccd43b 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -536,6 +536,8 @@ struct nfsd4_copy {
>> struct file *fh_src;
>> struct file *fh_dst;
>> struct net *net;
>> + struct nfs4_stid *stid;
>> + struct nfs4_cp_state *cps;
>> };
>>
>> struct nfsd4_seek {
>> --
>> 1.8.3.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
