From 04293bef6ccd2b9ca3db53906b63c952e235cdb4 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Wed, 5 Jan 2022 20:24:40 +0100 Subject: [PATCH] librbd: restore diff-iterate include_parent functionality in fast-diff mode Commit 4429ed4f3f4c ("librbd: switch diff iterate API to use new snaps list dispatch methods") removed the recursive execute() call. The new list_snaps method does indeed handle parent diffs internally but it is not used in fast-diff mode. Nothing changed there -- we still need to load the parent object map, calculate parent object_diff_state, etc. Fixes: https://tracker.ceph.com/issues/53787 Signed-off-by: Ilya Dryomov --- src/librbd/api/DiffIterate.cc | 31 ++++++++++++++-- src/test/librbd/test_librbd.cc | 67 ++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 3 deletions(-) diff --git a/src/librbd/api/DiffIterate.cc b/src/librbd/api/DiffIterate.cc index 8b0fa836f81..e015944444d 100644 --- a/src/librbd/api/DiffIterate.cc +++ b/src/librbd/api/DiffIterate.cc @@ -37,7 +37,6 @@ struct DiffContext { bool include_parent; uint64_t from_snap_id; uint64_t end_snap_id; - interval_set parent_diff; OrderedThrottle throttle; template @@ -150,6 +149,15 @@ private: } }; +int simple_diff_cb(uint64_t off, size_t len, int exists, void *arg) { + // it's possible for a discard to create a hole in the parent image -- ignore + if (exists) { + interval_set *diff = static_cast *>(arg); + diff->insert(off, len); + } + return 0; +} + } // anonymous namespace template @@ -237,6 +245,7 @@ int DiffIterate::execute() { int r; bool fast_diff_enabled = false; BitVector<2> object_diff_state; + interval_set parent_diff; if (m_whole_object) { C_SaferCond ctx; auto req = object_map::DiffRequest::create(&m_image_ctx, from_snap_id, @@ -250,6 +259,22 @@ int DiffIterate::execute() { } else { ldout(cct, 5) << "fast diff enabled" << dendl; fast_diff_enabled = true; + + // check parent overlap only if we are comparing to the beginning of time + if (m_include_parent && from_snap_id == 0) { + std::shared_lock image_locker{m_image_ctx.image_lock}; + uint64_t overlap = 0; + m_image_ctx.get_parent_overlap(m_image_ctx.snap_id, &overlap); + if (m_image_ctx.parent && overlap > 0) { + ldout(cct, 10) << " first getting parent diff" << dendl; + DiffIterate diff_parent(*m_image_ctx.parent, {}, nullptr, 0, overlap, + true, true, &simple_diff_cb, &parent_diff); + r = diff_parent.execute(); + if (r < 0) { + return r; + } + } + } } } @@ -282,13 +307,13 @@ int DiffIterate::execute() { const uint64_t object_no = extents.front().objectno; uint8_t diff_state = object_diff_state[object_no]; if (diff_state == object_map::DIFF_STATE_HOLE && - from_snap_id == 0 && !diff_context.parent_diff.empty()) { + from_snap_id == 0 && !parent_diff.empty()) { // no data in child object -- report parent diff instead for (auto& oe : extents) { for (auto& be : oe.buffer_extents) { interval_set o; o.insert(off + be.first, be.second); - o.intersection_of(diff_context.parent_diff); + o.intersection_of(parent_diff); ldout(cct, 20) << " reporting parent overlap " << o << dendl; for (auto e = o.begin(); e != o.end(); ++e) { r = m_callback(e.get_start(), e.get_len(), true, diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc index dd5f492a47c..766a977367f 100644 --- a/src/test/librbd/test_librbd.cc +++ b/src/test/librbd/test_librbd.cc @@ -4518,6 +4518,73 @@ TYPED_TEST(DiffIterateTest, DiffIterateRegression6926) ASSERT_EQ(static_cast(0), extents.size()); } +TYPED_TEST(DiffIterateTest, DiffIterateParent) +{ + REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + + librados::IoCtx ioctx; + ASSERT_EQ(0, this->_rados.ioctx_create(this->m_pool_name.c_str(), ioctx)); + + { + librbd::RBD rbd; + librbd::Image image; + int order = 22; + std::string name = this->get_temp_image_name(); + ssize_t size = 20 << 20; + + ASSERT_EQ(0, create_image_pp(rbd, ioctx, name.c_str(), size, &order)); + ASSERT_EQ(0, rbd.open(ioctx, image, name.c_str(), NULL)); + + uint64_t features; + ASSERT_EQ(0, image.features(&features)); + uint64_t object_size = 0; + if (this->whole_object) { + object_size = 1 << order; + } + + ceph::bufferlist bl; + bl.append(std::string(size, '1')); + ASSERT_EQ(size, image.write(0, size, bl)); + ASSERT_EQ(0, image.snap_create("snap")); + ASSERT_EQ(0, image.snap_protect("snap")); + + std::string clone_name = this->get_temp_image_name(); + ASSERT_EQ(0, rbd.clone(ioctx, name.c_str(), "snap", ioctx, + clone_name.c_str(), features, &order)); + librbd::Image clone; + ASSERT_EQ(0, rbd.open(ioctx, clone, clone_name.c_str(), NULL)); + + std::vector extents; + ASSERT_EQ(0, clone.diff_iterate2(NULL, 0, size, true, this->whole_object, + vector_iterate_cb, &extents)); + ASSERT_EQ(5u, extents.size()); + ASSERT_EQ(diff_extent(0, 4194304, true, object_size), extents[0]); + ASSERT_EQ(diff_extent(4194304, 4194304, true, object_size), extents[1]); + ASSERT_EQ(diff_extent(8388608, 4194304, true, object_size), extents[2]); + ASSERT_EQ(diff_extent(12582912, 4194304, true, object_size), extents[3]); + ASSERT_EQ(diff_extent(16777216, 4194304, true, object_size), extents[4]); + extents.clear(); + + ASSERT_EQ(0, clone.resize(size / 2)); + ASSERT_EQ(0, clone.resize(size)); + ASSERT_EQ(1, clone.write(size - 1, 1, bl)); + + ASSERT_EQ(0, clone.diff_iterate2(NULL, 0, size, true, this->whole_object, + vector_iterate_cb, &extents)); + ASSERT_EQ(4u, extents.size()); + ASSERT_EQ(diff_extent(0, 4194304, true, object_size), extents[0]); + ASSERT_EQ(diff_extent(4194304, 4194304, true, object_size), extents[1]); + ASSERT_EQ(diff_extent(8388608, 2097152, true, object_size), extents[2]); + // hole (parent overlap = 10M) followed by copyup'ed object + ASSERT_EQ(diff_extent(16777216, 4194304, true, object_size), extents[3]); + + ASSERT_PASSED(this->validate_object_map, image); + ASSERT_PASSED(this->validate_object_map, clone); + } + + ioctx.close(); +} + TYPED_TEST(DiffIterateTest, DiffIterateIgnoreParent) { REQUIRE_FEATURE(RBD_FEATURE_LAYERING); -- 2.39.5