From 3ff21e43bdfb61d1e47e0dbcd93c914071651673 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Fri, 5 Feb 2021 10:41:30 -0500 Subject: [PATCH] librbd/deep-copy: object-copy state machine must update object map If there was no data to copy, the object-copy state machine was bypassing the object-map update states and prematurely completing. Since the object-map is default-initialized to all non-existent objects, this results in incorrect state for OBJECT_EXISTS_CLEAN objects. This commit was derived from ca0b9bfc28ef7287ca139ca9640c876223eda87b Signed-off-by: Jason Dillaman --- src/librbd/deep_copy/ObjectCopyRequest.cc | 123 +++++++++++----- src/librbd/deep_copy/ObjectCopyRequest.h | 2 + .../deep_copy/test_mock_ObjectCopyRequest.cc | 131 +++++++++++++++--- 3 files changed, 202 insertions(+), 54 deletions(-) diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc index dd5da5f31a16d..01c4aeddba6de 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.cc +++ b/src/librbd/deep_copy/ObjectCopyRequest.cc @@ -40,6 +40,7 @@ namespace deep_copy { using librbd::util::create_context_callback; using librbd::util::create_rados_callback; +using librbd::util::get_image_ctx; template ObjectCopyRequest::ObjectCopyRequest(I *src_image_ctx, @@ -61,14 +62,17 @@ ObjectCopyRequest::ObjectCopyRequest(I *src_image_ctx, ceph_assert(!m_snap_map.empty()); m_src_async_op = new io::AsyncOperation(); - m_src_async_op->start_op(*util::get_image_ctx(m_src_image_ctx)); + m_src_async_op->start_op(*get_image_ctx(m_src_image_ctx)); m_src_io_ctx.dup(m_src_image_ctx->data_ctx); m_dst_io_ctx.dup(m_dst_image_ctx->data_ctx); m_dst_oid = m_dst_image_ctx->get_object_name(dst_object_number); - ldout(m_cct, 20) << "dst_oid=" << m_dst_oid << dendl; + ldout(m_cct, 20) << "dst_oid=" << m_dst_oid << ", " + << "src_snap_id_start=" << m_src_snap_id_start << ", " + << "dst_snap_id_start=" << m_dst_snap_id_start << ", " + << "snap_map=" << m_snap_map << dendl; compute_src_object_extents(); } @@ -259,7 +263,7 @@ void ObjectCopyRequest::send_read_from_parent() { auto ctx = create_context_callback< ObjectCopyRequest, &ObjectCopyRequest::handle_read_from_parent>(this); auto comp = io::AioCompletion::create_and_start( - ctx, util::get_image_ctx(m_src_image_ctx->parent), io::AIO_TYPE_READ); + ctx, get_image_ctx(m_src_image_ctx->parent), io::AIO_TYPE_READ); ldout(m_cct, 20) << "completion " << comp << ", extents " << image_extents << dendl; @@ -301,20 +305,21 @@ void ObjectCopyRequest::handle_read_from_parent(int r) { compute_dst_object_may_exist(); compute_zero_ops(); - if (m_write_ops.empty()) { - // nothing to copy - finish(-ENOENT); - return; - } - send_update_object_map(); - return; } template void ObjectCopyRequest::send_update_object_map() { if (!m_dst_image_ctx->test_features(RBD_FEATURE_OBJECT_MAP) || m_dst_object_state.empty()) { + if (m_write_ops.empty()) { + // no data to copy or truncate/zero. only the copyup state machine cares + // about whether the object exists or not, and it always copies from + // snap id 0. + finish(m_src_snap_id_start > 0 ? 0 : -ENOENT); + return; + } + send_write_object(); return; } @@ -380,12 +385,7 @@ void ObjectCopyRequest::handle_update_object_map(int r) { return; } - if (!m_dst_object_state.empty()) { - send_update_object_map(); - return; - } - - send_write_object(); + send_update_object_map(); } template @@ -601,14 +601,68 @@ void ObjectCopyRequest::compute_read_ops() { m_src_image_ctx->image_lock.unlock_shared(); librados::snap_t src_copy_point_snap_id = m_snap_map.rbegin()->first; - bool prev_exists = (hide_parent || m_src_snap_id_start > 0); + bool prev_exists = hide_parent; uint64_t prev_end_size = prev_exists ? m_src_image_ctx->layout.object_size : 0; + + if (m_src_snap_id_start > 0) { + // determine if the src object exists within the dst object at the + // specified start snapshot so we can properly compute its object + // map state later + interval_set diff; + uint64_t end_size; + bool exists; + librados::snap_t clone_end_snap_id; + calc_snap_set_diff(m_cct, m_snap_set, 0, m_src_snap_id_start, &diff, + &end_size, &exists, &clone_end_snap_id, + &m_read_whole_object); + if (m_read_whole_object) { + exists = true; + end_size = m_src_image_ctx->layout.object_size; + } + + ldout(m_cct, 20) << "start_src_snap_id=0, " + << "end_src_snap_id=" << m_src_snap_id_start << ", " + << "end_size=" << end_size << ", " + << "exists=" << exists << dendl; + if (exists) { + prev_exists = true; + prev_end_size = end_size; + + for (auto& [dst_object_offset, src_object_extent] : m_src_object_extents) { + if (src_object_extent.object_no != m_src_ono || + src_object_extent.offset + 1 > end_size) { + // src extent maps to a different object or an extent that starts + // after the current object end_size + continue; + } + + // trim the src extent length to the corresponding end size + auto src_object_extent_length = std::min( + src_object_extent.length, end_size - src_object_extent.offset); + auto dst_object_extent_length = dst_object_offset + + src_object_extent_length; + + ldout(m_cct, 20) << "src_end_size=" << src_object_extent_length << ", " + << "dst_end_size=" << dst_object_extent_length + << dendl; + m_dst_object_size = std::max(m_dst_object_size.value_or(0), + dst_object_extent_length); + } + } + } + librados::snap_t start_src_snap_id = m_src_snap_id_start; for (auto &pair : m_snap_map) { ceph_assert(!pair.second.empty()); librados::snap_t end_src_snap_id = pair.first; + if (start_src_snap_id >= end_src_snap_id) { + // skip any snapshots prior to the requested start position + ldout(m_cct, 20) << "skipping src snap id " << end_src_snap_id << dendl; + continue; + } + librados::snap_t end_dst_snap_id = pair.second.front(); interval_set diff; @@ -865,8 +919,9 @@ template void ObjectCopyRequest::compute_zero_ops() { ldout(m_cct, 20) << dendl; + bool object_exists = (m_src_snap_id_start > 0 && !!m_dst_object_size); bool fast_diff = m_dst_image_ctx->test_features(RBD_FEATURE_FAST_DIFF); - uint64_t prev_end_size = 0; + uint64_t prev_end_size = m_dst_object_size.value_or(0); m_src_image_ctx->image_lock.lock_shared(); bool hide_parent = (m_src_image_ctx->parent != nullptr); @@ -882,9 +937,10 @@ void ObjectCopyRequest::compute_zero_ops() { auto dst_may_exist_it = m_dst_object_may_exist.find(dst_snap_seq); ceph_assert(dst_may_exist_it != m_dst_object_may_exist.end()); - if (!dst_may_exist_it->second && prev_end_size > 0) { + if (!dst_may_exist_it->second && object_exists) { ldout(m_cct, 5) << "object DNE for snap_id: " << dst_snap_seq << dendl; m_write_ops[src_snap_seq].emplace_back(COPY_OP_TYPE_REMOVE, 0, 0, 0); + object_exists = false; prev_end_size = 0; continue; } @@ -912,11 +968,6 @@ void ObjectCopyRequest::compute_zero_ops() { if (overlap == 0) { ldout(m_cct, 20) << "no parent overlap" << dendl; hide_parent = false; - } else if (src_snap_seq == m_dst_zero_interval.begin()->first) { - for (auto e : image_extents) { - prev_end_size += e.second; - } - ceph_assert(prev_end_size <= m_dst_image_ctx->layout.object_size); } } } @@ -928,6 +979,7 @@ void ObjectCopyRequest::compute_zero_ops() { if (iter != m_write_ops.end()) { for (auto ©_op : iter->second) { for (auto &e : copy_op.dst_extent_map) { + object_exists = true; end_size = std::max(end_size, e.first + e.second); } } @@ -940,15 +992,18 @@ void ObjectCopyRequest::compute_zero_ops() { m_write_ops[src_snap_seq] .emplace_back(COPY_OP_TYPE_REMOVE_TRUNC, 0, 0, 0); ldout(m_cct, 20) << "COPY_OP_TYPE_REMOVE_TRUNC" << dendl; + object_exists = true; } else if (z.get_start() < prev_end_size) { if (z.get_start() == 0) { m_write_ops[src_snap_seq] .emplace_back(COPY_OP_TYPE_REMOVE, 0, 0, 0); ldout(m_cct, 20) << "COPY_OP_TYPE_REMOVE" << dendl; + object_exists = false; } else { m_write_ops[src_snap_seq] .emplace_back(COPY_OP_TYPE_TRUNC, 0, z.get_start(), 0); ldout(m_cct, 20) << "COPY_OP_TYPE_TRUNC " << z.get_start() << dendl; + object_exists = true; } } end_size = std::min(end_size, z.get_start()); @@ -958,17 +1013,23 @@ void ObjectCopyRequest::compute_zero_ops() { .emplace_back(COPY_OP_TYPE_ZERO, 0, z.get_start(), z.get_len()); ldout(m_cct, 20) << "COPY_OP_TYPE_ZERO " << z.get_start() << "~" << z.get_len() << dendl; + object_exists = true; } } - ldout(m_cct, 20) << "src_snap_seq=" << src_snap_seq << ", end_size=" - << end_size << dendl; - if (end_size > 0 || hide_parent) { - m_dst_object_state[src_snap_seq] = OBJECT_EXISTS; - if (fast_diff && end_size == prev_end_size && - m_write_ops[src_snap_seq].empty()) { - m_dst_object_state[src_snap_seq] = OBJECT_EXISTS_CLEAN; + + uint8_t dst_object_map_state = OBJECT_NONEXISTENT; + if (object_exists) { + dst_object_map_state = OBJECT_EXISTS; + if (fast_diff && m_write_ops[src_snap_seq].empty()) { + dst_object_map_state = OBJECT_EXISTS_CLEAN; } + m_dst_object_state[src_snap_seq] = dst_object_map_state; } + + ldout(m_cct, 20) << "dst_snap_seq=" << dst_snap_seq << ", " + << "end_size=" << end_size << ", " + << "dst_object_map_state=" + << static_cast(dst_object_map_state) << dendl; prev_end_size = end_size; } } diff --git a/src/librbd/deep_copy/ObjectCopyRequest.h b/src/librbd/deep_copy/ObjectCopyRequest.h index c39c33b9b6326..ce4b5d600c32a 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.h +++ b/src/librbd/deep_copy/ObjectCopyRequest.h @@ -13,6 +13,7 @@ #include "librbd/io/Types.h" #include #include +#include #include class Context; @@ -172,6 +173,7 @@ private: std::map> m_dst_zero_interval; std::map m_dst_object_state; std::map m_dst_object_may_exist; + std::optional m_dst_object_size = std::nullopt; bufferlist m_read_from_parent_data; io::AsyncOperation* m_src_async_op = nullptr; diff --git a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc index dc1c4f323c1af..cb64be8b2d3c0 100644 --- a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc @@ -11,6 +11,7 @@ #include "librbd/Operations.h" #include "librbd/api/Image.h" #include "librbd/deep_copy/ObjectCopyRequest.h" +#include "librbd/deep_copy/Utils.h" #include "librbd/io/ImageRequest.h" #include "librbd/io/ImageRequestWQ.h" #include "librbd/io/ReadResult.h" @@ -116,6 +117,7 @@ public: ContextWQ *m_work_queue; SnapMap m_snap_map; + SnapSeqs m_snap_seqs; std::vector m_src_snap_ids; std::vector m_dst_snap_ids; @@ -206,12 +208,18 @@ public: librbd::MockTestImageCtx &mock_src_image_ctx, librbd::MockTestImageCtx &mock_dst_image_ctx, librados::snap_t src_snap_id_start, + librados::snap_t src_snap_id_end, librados::snap_t dst_snap_id_start, Context *on_finish) { + SnapMap snap_map; + util::compute_snap_map(mock_dst_image_ctx.cct, src_snap_id_start, + src_snap_id_end, m_dst_snap_ids, m_snap_seqs, + &snap_map); + expect_get_object_name(mock_dst_image_ctx); return new MockObjectCopyRequest(&mock_src_image_ctx, &mock_dst_image_ctx, src_snap_id_start, dst_snap_id_start, - m_snap_map, 0, false, nullptr, on_finish); + snap_map, 0, false, nullptr, on_finish); } void expect_set_snap_read(librados::MockTestMemIoCtxImpl &mock_io_ctx, @@ -349,6 +357,7 @@ public: m_snap_map.rbegin()->second.end()); } m_snap_map[src_snap_id] = dst_snap_ids; + m_snap_seqs[src_snap_id] = dst_snap_id; m_src_snap_ids.push_back(src_snap_id); m_dst_snap_ids.push_back(dst_snap_id); @@ -480,8 +489,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, DNE) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -513,8 +522,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Write) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -560,8 +569,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadMissingStaleSnapSet) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -633,8 +642,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadMissingUpToDateSnapMap) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -669,8 +678,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadError) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -704,8 +713,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteError) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -759,8 +768,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -829,8 +838,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Trim) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -886,8 +895,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Remove) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -941,8 +950,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ObjectMapUpdateError) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, - mock_dst_image_ctx, 0, 0, - &ctx); + mock_dst_image_ctx, 0, + CEPH_NOSNAP, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_src_io_ctx(get_mock_io_ctx( request->get_src_io_ctx())); @@ -1000,8 +1009,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) { // map should begin after src start and src end's dst snap seqs should // point to HEAD revision - m_snap_map.erase(src_snap_id_start); - m_snap_map[m_src_image_ctx->snaps[0]][0] = CEPH_NOSNAP; + m_snap_seqs.rbegin()->second = CEPH_NOSNAP; + m_dst_snap_ids.pop_back(); librbd::MockTestImageCtx mock_src_image_ctx(*m_src_image_ctx); librbd::MockTestImageCtx mock_dst_image_ctx(*m_dst_image_ctx); @@ -1019,6 +1028,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) { MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, src_snap_id_start, + CEPH_NOSNAP, dst_snap_id_start, &ctx); @@ -1057,5 +1067,80 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) { ASSERT_EQ(0, compare_objects()); } +TEST_F(TestMockDeepCopyObjectCopyRequest, Incremental) { + librbd::MockTestImageCtx mock_src_image_ctx(*m_src_image_ctx); + librbd::MockTestImageCtx mock_dst_image_ctx(*m_dst_image_ctx); + + librbd::MockExclusiveLock mock_exclusive_lock; + prepare_exclusive_lock(mock_dst_image_ctx, mock_exclusive_lock); + + librbd::MockObjectMap mock_object_map; + mock_dst_image_ctx.object_map = &mock_object_map; + + expect_op_work_queue(mock_src_image_ctx); + expect_test_features(mock_dst_image_ctx); + expect_get_object_count(mock_dst_image_ctx); + + // scribble some data + interval_set one; + scribble(m_src_image_ctx, 10, 102400, &one); + ASSERT_EQ(0, create_snap("snap1")); + mock_dst_image_ctx.snaps = m_dst_image_ctx->snaps; + + InSequence seq; + + C_SaferCond ctx1; + auto request1 = create_request(mock_src_image_ctx, mock_dst_image_ctx, + 0, m_src_snap_ids[0], 0, &ctx1); + + librados::MockTestMemIoCtxImpl &mock_src_io_ctx1(get_mock_io_ctx( + request1->get_src_io_ctx())); + expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx1, 0); + + expect_set_snap_read(mock_src_io_ctx1, m_src_snap_ids[0]); + expect_sparse_read(mock_src_io_ctx1, 0, one.range_end(), 0); + + expect_start_op(mock_exclusive_lock); + expect_update_object_map(mock_dst_image_ctx, mock_object_map, + m_dst_snap_ids[0], OBJECT_EXISTS, 0); + + librados::MockTestMemIoCtxImpl &mock_dst_io_ctx1(get_mock_io_ctx( + request1->get_dst_io_ctx())); + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx1, 0, one.range_end(), {0, {}}, 0); + + request1->send(); + ASSERT_EQ(0, ctx1.wait()); + + // clean (no-updates) snapshots + ASSERT_EQ(0, create_snap("snap2")); + ASSERT_EQ(0, create_snap("snap3")); + mock_dst_image_ctx.snaps = m_dst_image_ctx->snaps; + + C_SaferCond ctx2; + auto request2 = create_request(mock_src_image_ctx, mock_dst_image_ctx, + m_src_snap_ids[0], m_src_snap_ids[2], + m_dst_snap_ids[0], &ctx2); + + librados::MockTestMemIoCtxImpl &mock_src_io_ctx2(get_mock_io_ctx( + request2->get_src_io_ctx())); + expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx2, 0); + + expect_start_op(mock_exclusive_lock); + expect_update_object_map(mock_dst_image_ctx, mock_object_map, + m_dst_snap_ids[1], + is_fast_diff(mock_dst_image_ctx) ? + OBJECT_EXISTS_CLEAN : OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); + expect_update_object_map(mock_dst_image_ctx, mock_object_map, + m_dst_snap_ids[2], + is_fast_diff(mock_dst_image_ctx) ? + OBJECT_EXISTS_CLEAN : OBJECT_EXISTS, 0); + + request2->send(); + ASSERT_EQ(0, ctx2.wait()); + ASSERT_EQ(0, compare_objects()); +} + } // namespace deep_copy } // namespace librbd -- 2.39.5