[RFC] librados: support copy-less writes in librados C API #12216
Conversation
|
Looks like a good thing (tm). |
branch-predictor
force-pushed
the
bp-librados-nocopy-write
branch
from
d646143
to
41b445e
Compare
This commit introduces rados_write2(), rados_write_full2(), rados_writesame2(), rados_append2() and rados_setxattr2() which do not copy user buffers into temporary bufferptrs, reducing memory and CPU usage, at expense of requiring that provied buffers don't change through entire write/append, which should perform synchronously anyway. Otherwise these "2" function perform as their non-"2" equivalents. Signed-off-by: Piotr Dałek <git@predictor.org.pl>
branch-predictor
force-pushed
the
bp-librados-nocopy-write
branch
from
41b445e
to
284841c
Compare
This commit introduces rados_aio_write2(), rados_aio_write_full2(), rados_aio_writesame2(), rados_aio_append2() and rados_aio_setxattr2() which do not copy user buffers into temporary bufferptrs, reducing memory and CPU usage, at expense of requiring that provied buffers don't change through entire write/append. Otherwise these "2" function perform as their non-"2" equivalents. Signed-off-by: Piotr Dałek <git@predictor.org.pl>
branch-predictor
force-pushed
the
bp-librados-nocopy-write
branch
from
607e361
to
ba5a232
Compare
branch-predictor
changed the title
|
great! |
|
I'm not sure "2" is a good idea to indicate this? |
|
On Tue, Nov 29, 2016 at 06:24:03PM -0800, Haomai Wang wrote:
I'm not sure "2" is a good idea to indicate this?
That's an RFC, so I'm open to suggestions. For sure it cannot be another
overload or same function with extra arg (breaks API compatibility).
Also I doubt if we really need the "2" versions for sync writes -
assuming they work 100% synchronusly, the possibility of freeing/changing
buf during sync write is recduced to deliberate race conditions in user code.
|
| * @returns 0 on success, negative error code on failure | ||
| */ | ||
| CEPH_RADOS_API int rados_write2(rados_ioctx_t io, const char *oid, | ||
| char *buf, size_t len, uint64_t off); |
There was a problem hiding this comment.
Perhaps rados_write_nc would be more obvious.
| bufferlist bl; | ||
| bufferptr bp = buffer::create_static(len, buf); | ||
| bl.push_back(bp); | ||
| int retval = ctx->write(oid, bl, len, off); |
There was a problem hiding this comment.
This looks competently done and reasonable.
|
I'll add these to the Rust bindings as soon as they land. Nice work! |
|
Probably need to ensure that this won't hit the same messenger race condition that was pointed out here [1] |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
@adamemerson should this be stale? |
|
@branch-predictor Is this something we still need/was there something blocking this moving forward, or just in need of review/testing? |
|
@adamemerson See my comment above -- if you pass wrapped memory pointers and those pointers get stuck in the messenger layer, boom. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution! |
These commits introduce rados_write2(), rados_write_full2(), rados_writesame2(), rados_append2() and rados_setxattr2(), rados_aio_write2(), rados_aio_write_full2(), rados_aio_writesame2(), rados_aio_append2() and rados_aio_setxattr2() which do not copy user buffers into temporary bufferptrs, reducing memory and CPU usage, at expense of requiring that provided buffers don't change through entire write/append. Otherwise these "2" function perform exactly like their non-"2" equivalents.
Signed-off-by: Piotr Dałek git@predictor.org.pl