Skip to content

Commit

Permalink
librbd: reduce buffer copy via static ptr
Browse files Browse the repository at this point in the history
Signed-off-by: Haomai Wang <haomai@xsky.com>
  • Loading branch information
yuyuyu101 committed Jan 27, 2016
1 parent d46409a commit 794b49b
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/librbd/AioImageRequest.cc
Expand Up @@ -309,7 +309,8 @@ void AioImageWrite::assemble_extent(const ObjectExtent &object_extent,
bufferlist *bl) {
for (Extents::const_iterator q = object_extent.buffer_extents.begin();
q != object_extent.buffer_extents.end(); ++q) {
bl->append(m_buf + q->first, q->second);;
auto p = buffer::create_static(q->second, m_buf + q->first);

This comment has been minimized.

Copy link
@liewegas

liewegas Jan 27, 2016

In order to do this we have to be completely certain that all references to this buffer will be gone by the time the write is complete. I think that may not be strictly true... for example,

  • send write to osd.0
  • crush layout changes
  • send write to osd.1, write completes, Aio write goes away
  • message finally gets around to writing message to osd.0 socket, referencing unallocated memory

This comment has been minimized.

Copy link
@yuyuyu101

yuyuyu101 Jan 27, 2016

Author Owner

....Nice...I hope we can promote a way to solve this rare case... Extra copy always annoy me...

This comment has been minimized.

Copy link
@dillaman

dillaman Jan 27, 2016

When I tested this change, it introduced several use-after-free conditions. See PR ceph#5898 which replaces the C-style buffers with bufferlist to safely avoid copying. Of course, it doesn't handle the case mentioned above when using a C-array for writes since I didn't know such a thing was possible.

This comment has been minimized.

Copy link
@yuyuyu101

yuyuyu101 Jan 28, 2016

Author Owner

Actually I'm thinking if we could expose a buffer::create_ptr alike api to C-api, so caller used this api to create buffer and store lots of things. Hmm, though it's not a very comfortable usage to caller.....

This comment has been minimized.

Copy link
@yuyuyu101

yuyuyu101 Jan 28, 2016

Author Owner

@liewegas do you think what if messenger could support cancel_message api? which may solve some problems.... And I guess it will also help to other cases

bl->append(std::move(p));;
}
}

Expand Down Expand Up @@ -366,7 +367,7 @@ AioObjectRequest *AioImageWrite::create_object_request(
AioObjectWrite *req = new AioObjectWrite(&m_image_ctx,
object_extent.oid.name,
object_extent.objectno,
object_extent.offset, bl,
object_extent.offset, std::move(bl),
snapc, on_finish);
req->set_op_flags(m_op_flags);
return req;
Expand Down

0 comments on commit 794b49b

Please sign in to comment.