[Gluster-devel] [puzzle] readv operation allocate iobuf twice

[Mohammed Rafi K C]

On 07/15/2016 12:44 AM, Zhengping Zhou wrote:
> Could we just not support user's preset rsp buffer for function
> rpc_clnt_submit, which means remove some relevant parameters of
> function rpc_clnt_submit such like rsphdr, rsphdr_count, rsp_payload,
> rsp_payload_count, rsp_iobref.
>
> The reasons as follow:
>
> 1.We not only should put rsp info to saved_frame, but also should
> duplicate iovec rsp.payload. So we should GF_FREE(
> saved_frame.rsp.rsp_payload)  before mem_put(saved_frame) . It seem's
> ugly.
>     wind:
>            saved_frame->rsp = *rsp (this rsp is a pointer to local parameter);
>            saved_frame->rsp.rsp_payload = iov_dup (rsp.rsp_payload,
> rsp.rsp_payload_count);
>     unwind:
>            GF_FREE (saved_frame->rsp.rsp_payload);
>            memput (saved_frame);
I agree with you. With current code base, it doesn't seems to be helping
much.

But we when we use libgfapi, we could implement zero copy read by
changing the iobuf_get in client3_3_readv, so that the user given
address can be passed as rsp buffer, and if we could give the data in
the same memory , that will help to reduce one in memory copy.

The same can also implement by giving the gluster memory to the user as
a response to libgfapi read.

Apart from this, I don't see any benefit for allocating the memory from
client3_3_readv and giving in rsp struct.


Regards
Rafi KC


>
> 2.If we finish the patch as the way mentioned above, and what about
> rsphdr ? According to the note of rpc_clnt_submit, the rsp_hdr and
> rsp_payload both could preset by users. But we even can't get
> saved_frame before we use rsphdr in socket read state machine, because
> we need rsphdr's content to get saved_frame.
>
> 3.I can't figure out the benefit of presetting rsphdr and rsp_payload.
>
> 2016-07-12 12:38 GMT+08:00 Raghavendra Gowdappa <rgowdapp at redhat.com>:
>>
>> ----- Original Message -----
>>> From: "Zhengping Zhou" <johnzzpcrystal at gmail.com>
>>> To: gluster-devel at gluster.org
>>> Sent: Tuesday, July 12, 2016 9:28:01 AM
>>> Subject: [Gluster-devel] [puzzle] readv operation allocate iobuf twice
>>>
>>> Hi all:
>>>
>>>     It is a puzzle to me that we  allocate rsp buffers for rspond
>>> content in function client3_3_readv, but these rsp parameters hasn't
>>> ever been saved to struct saved_frame in submit procedure.
>> Good catch :). We were aware of this issue, but the fix wasn't prioritized. Can you please file a bug on this? If you want to send a fix (which essentially stores the rsp payload ptr in saved-frame and passes it down during rpc_clnt_fill_request_info - as part of handling RPC_TRANSPORT_MAP_XID_REQUEST event in rpc-clnt), please post a patch to gerrit and I'll accept it. If you don't have bandwidth, one of us can send out a fix too.
>>
>> Again, thanks for the effort :).
>>
>> regards,
>> Raghavendra
>>
>>> Which means
>>> the iobuf will reallocated by transport layer in function
>>> __socket_read_accepted_successful_reply.
>>>     According to  the commnet of fucntion rpc_clnt_submit :
>>>     1. Both @rsp_hdr and @rsp_payload are optional.
>>>     2. The user of rpc_clnt_submit, if wants response hdr and payload in its
>>>     own
>>>         buffers, then it has to populate @rsphdr and @rsp_payload.
>>>         ....
>>>     The rsp_payload  is optional, ransport layer will not reallocate
>>> rsp buffers if
>>> it populated. But the fact is readv operation will allocate rsp buffer twice.
>>>
>>> Thanks
>>> Zhengping
>>> _______________________________________________
>>> Gluster-devel mailing list
>>> Gluster-devel at gluster.org
>>> http://www.gluster.org/mailman/listinfo/gluster-devel
>>>
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel