[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. 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@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [puzzle] readv operation allocate iobuf twice
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 : >> >> - Original Message ----- >>> From: "Zhengping Zhou" >>> To: gluster-devel@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@gluster.org >>> http://www.gluster.org/mailman/listinfo/gluster-devel >>> > ___ > Gluster-devel mailing list > Gluster-devel@gluster.org > http://www.gluster.org/mailman/listinfo/gluster-devel ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [puzzle] readv operation allocate iobuf twice
- Original Message - > From: "Zhengping Zhou" > To: gluster-devel@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@gluster.org > http://www.gluster.org/mailman/listinfo/gluster-devel > ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [puzzle] readv operation allocate iobuf twice
I have all ready filed a bug with bugid 1354205, but my current patch still has problem in my test environment, I'll check it out and post later. 2016-07-12 12:38 GMT+08:00 Raghavendra Gowdappa : > > > - Original Message - >> From: "Zhengping Zhou" >> To: gluster-devel@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@gluster.org >> http://www.gluster.org/mailman/listinfo/gluster-devel >> ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [puzzle] readv operation allocate iobuf twice
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); 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 : > > > - Original Message - >> From: "Zhengping Zhou" >> To: gluster-devel@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@gluster.org >> http://www.gluster.org/mailman/listinfo/gluster-devel >> ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel