New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

librbd: create fewer empty objects during copyup #12326

Merged
merged 4 commits into from Jan 23, 2017

Conversation

Projects
None yet
3 participants
@vshankar
Contributor

vshankar commented Dec 5, 2016

This is based out of Doug's (@fullerdj) work (PR #9329)
as an attempt to avoid creating empty objects when
flattening an image and otherwise whenever unnecessary.

This gives good optimization benefit when a parent image
is sparsely populated. Moreover, this change is required
for correct behavior when checking disk usage of a clone
(which used to report fully allocated image due to all,
including empty objects being created during flatten).

@vshankar

This comment has been minimized.

Contributor

vshankar commented Dec 11, 2016

Required ceph-qa-suite change: ceph/ceph-qa-suite#1304

@vshankar vshankar changed the title from [DNM] Create few empty objects to Create few empty objects Dec 11, 2016

@dillaman dillaman changed the title from Create few empty objects to librbd: create fewer empty objects during copyup Jan 11, 2017

virtual void handle_write_guard();
void send_pre_object_map_update();

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

The state diagram needs to be updated to match the new flow

@@ -582,7 +588,8 @@ void AioObjectWrite::send_write() {
<< " object exist " << m_object_exist
<< " write_full " << write_full << dendl;
if (write_full && !has_parent()) {
send_write_op(false);
m_guard = false;
send_pre_object_map_update();

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

Just for clarity sake, could we remove the else guard around AbstractAioObjectWrite::send_write and just fall through here? In AbstractAioObjectWrite::send_write, remove the setting of m_guard to send and allow it to just send the object map update.

@@ -238,21 +250,21 @@ class AbstractAioObjectWrite : public AioObjectRequest<> {
uint64_t m_snap_seq;
std::vector<librados::snap_t> m_snaps;
bool m_object_exist;
bool m_guard;

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

Nit: default initialize this to true here

@@ -496,7 +501,8 @@ void AbstractAioObjectWrite::send_write() {
m_state = LIBRBD_AIO_WRITE_GUARD;
handle_write_guard();
} else {
send_write_op(true);
m_guard = true;

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

Nit: remove this assignment here and rely on the default initialization

@@ -598,8 +605,10 @@ void AioObjectRemove::guard_write() {
void AioObjectRemove::send_write() {
ldout(m_ictx->cct, 20) << "send_write " << this << " " << m_oid << " "
<< m_object_off << "~" << m_object_len << dendl;
send_write_op(true);
m_guard = true;

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

Nit: remove and rely on the default initialization

bool CopyupRequest::is_nop() {
bool noop = true;
for (const AioObjectRequest<> *req : m_pending_requests) {
const AioObjectWrite *wreq = dynamic_cast<const AioObjectWrite *>(req);

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

I'd eliminate the dynamic_cast and just add a new pure virtual method to the base object request class that returns true/false for whether or not it's an empty write op

@@ -181,6 +181,19 @@ bool CopyupRequest::send_copyup() {
return false;
}
bool CopyupRequest::is_nop() {

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

Nit: can we rename this to something like is_copy_up_required?

@@ -58,7 +58,8 @@ class CopyupRequest {
*/
enum State {
STATE_READ_FROM_PARENT,
STATE_OBJECT_MAP,
STATE_OBJECT_MAP_HEAD,

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

Update the state diagram

object_map_locker.unlock();
snap_locker.unlock();
owner_locker.unlock();
return send_object_map_snap();

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

State name doesn't really align here for the copy-on-read case since it is also updating the HEAD revision. Maybe keep the original name?

@@ -76,7 +76,7 @@ AioObjectRequest<I>::AioObjectRequest(ImageCtx *ictx, const std::string &oid,
Context *completion, bool hide_enoent)
: m_ictx(ictx), m_oid(oid), m_object_no(objectno), m_object_off(off),
m_object_len(len), m_snap_id(snap_id), m_completion(completion),
m_hide_enoent(hide_enoent) {
m_hide_enoent(hide_enoent), m_has_parent(false) {

This comment has been minimized.

@dillaman

dillaman Jan 12, 2017

Contributor

Nit: move the default initialization to the header

@@ -83,6 +83,13 @@ class AioObjectRequest : public AioObjectRequestHandle {
return m_has_parent;
}
virtual bool is_empty() const {

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Nit: perhaps rename to is_op_payload_empty or something less generic

This comment has been minimized.

@dillaman

dillaman Jan 20, 2017

Contributor

Not sure why GitHub is hiding this comment

This comment has been minimized.

@vshankar

vshankar Jan 21, 2017

Contributor

done.

vshankar added some commits Dec 1, 2016

librbd: Create few empty objects during copyup
This is based out of Doug's (@fullerdj) work (PR #9329)
as an attempt to avoid creating empty objects when
flattening an image and otherwise whenever unnecessary.

This gives good optimization benefit when a parent image
is sparsely populated. Moreover, this change is required
for correct behavior when checking disk usage of a clone
(which used to report fully allocated image due to all,
including empty objects being created during flatten).

Signed-off-by: Douglas Fuller dfuller@redhat.com
Signed-off-by: Venky Shankar <vshankar@redhat.com>
test / librbd: Write ones instead of zeroes to test images
Signed-off-by: Douglas Fuller <dfuller@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
test / librbd: create few empty objects during flatten
Fixes: http://tracker.ceph.com/issues/15028
Signed-off-by: Venky Shankar <vshankar@redhat.com>
librbd: make has_parent() prone to callers from copyup
This is required when CopyupRequest would need to invoke
pre_object_map_update() as part of upcoming changes to
create fewer child image objects whenever possible.

CopyupRequest constructor accepts image extents as an
rvalue forcing the caller to transfer ownership to it
and leaving the original variable in an unspecified
stated making has_parent() return incorrect state when
invoked from CopyupRequest. Therefore, introduce a
private tracking state that can be used in place of
checking emptiness of parent image extents.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 21, 2017

retest this please

@dillaman dillaman merged commit 20da91e into ceph:master Jan 23, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
@trociny

This comment has been minimized.

Contributor

trociny commented Jan 24, 2017

@dillaman @vshankar Observing on the master:

beton:~/ceph.ci/build% CEPH_LIB=./lib RBD_FEATURES=127 ./bin/unittest_librbd --gtest_filter=TestLibRBD.FlattenNoEmptyObjects
Note: Google Test filter = TestLibRBD.FlattenNoEmptyObjects
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from TestLibRBD
seed 25696
[ RUN      ] TestLibRBD.FlattenNoEmptyObjects
using new format!
/home/mgolub/ceph.ci/src/test/librbd/test_librbd.cc:3391: Failure
      Expected: 0
To be equal to: create_image_full(ioctx, parent_name.c_str(), size, &order, false, features)
      Which is: -22
[  FAILED  ] TestLibRBD.FlattenNoEmptyObjects (4 ms)
[----------] 1 test from TestLibRBD (5 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (15 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] TestLibRBD.FlattenNoEmptyObjects

 1 FAILED TEST
@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 24, 2017

@trociny Is that from your own machine? Can you increase the log level to find out why librbd failed to create an image?

@trociny

This comment has been minimized.

Contributor

trociny commented Jan 24, 2017

2017-01-24 15:12:30.326155 7f812bc7ea00 10 librbd: create name=image1, size=4194304, opts=[format=2, features=127, order=12, stripe_unit=65536, stripe_count=16]
2017-01-24 15:12:30.326276 7f812bc7ea00 20 librbd::image::CreateRequest: name=image1, id=0608674e9, size=4194304, features=127, order=12, stripe_unit=65536, stripe_count=16, journal_order=24, journal_splay_width=4, journal_pool=, data_pool=
2017-01-24 15:12:30.326283 7f812bc7ea00 20 librbd::image::CreateRequest: 0x55880bba5680 send
2017-01-24 15:12:30.326286 7f812bc7ea00 -1 librbd::image::CreateRequest: stripe unit is not a factor of the object size

order is set to 12 in FlattenNoEmptyObjects, stripe_unit is hardcoded to 65536 in create_image_full().

@trociny

This comment has been minimized.

Contributor

trociny commented Jan 24, 2017

It fails only for RBD_FEATURES=127 case, which our scripts do not test, but I test locally

@vshankar

This comment has been minimized.

Contributor

vshankar commented Jan 24, 2017

It fails only for RBD_FEATURES=127 case, which our scripts do not test, but I test locally

Looks like that needs to be fixed. I'll do that...

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment