From 96a001c2379de09277ccc17fd70d24c6d573aa84 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Thu, 28 Jan 2021 21:42:09 -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. Signed-off-by: Jason Dillaman (cherry picked from commit ca0b9bfc28ef7287ca139ca9640c876223eda87b) --- src/librbd/deep_copy/ObjectCopyRequest.cc | 76 ++++++----- .../deep_copy/test_mock_ObjectCopyRequest.cc | 124 +++++++++++++++--- 2 files changed, 149 insertions(+), 51 deletions(-) diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc index 3c8c596b19db1..d114a2457e007 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.cc +++ b/src/librbd/deep_copy/ObjectCopyRequest.cc @@ -29,6 +29,7 @@ namespace deep_copy { using librbd::util::create_async_context_callback; 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, @@ -50,14 +51,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; } template @@ -77,7 +81,7 @@ void ObjectCopyRequest::send_list_snaps() { snap_ids.reserve(1 + m_snap_map.size()); snap_ids.push_back(m_src_snap_id_start); for (auto& [src_snap_id, _] : m_snap_map) { - if (src_snap_id != snap_ids.front()) { + if (m_src_snap_id_start < src_snap_id) { snap_ids.push_back(src_snap_id); } } @@ -90,7 +94,7 @@ void ObjectCopyRequest::send_list_snaps() { *m_src_image_ctx, create_context_callback< ObjectCopyRequest, &ObjectCopyRequest::handle_list_snaps>(this)); auto aio_comp = io::AioCompletion::create_and_start( - ctx, util::get_image_ctx(m_src_image_ctx), io::AIO_TYPE_GENERIC); + ctx, get_image_ctx(m_src_image_ctx), io::AIO_TYPE_GENERIC); auto req = io::ImageDispatchSpec::create_list_snaps( *m_src_image_ctx, io::IMAGE_DISPATCH_LAYER_NONE, aio_comp, io::Extents{m_image_extents}, std::move(snap_ids), list_snaps_flags, @@ -123,12 +127,6 @@ void ObjectCopyRequest::send_read() { merge_write_ops(); compute_zero_ops(); - if (m_snapshot_sparse_bufferlist.empty()) { - // nothing to copy - finish(-ENOENT); - return; - } - send_update_object_map(); return; } @@ -163,7 +161,7 @@ void ObjectCopyRequest::send_read() { auto ctx = create_context_callback< ObjectCopyRequest, &ObjectCopyRequest::handle_read>(this); auto aio_comp = io::AioCompletion::create_and_start( - ctx, util::get_image_ctx(m_src_image_ctx), io::AIO_TYPE_READ); + ctx, get_image_ctx(m_src_image_ctx), io::AIO_TYPE_READ); auto req = io::ImageDispatchSpec::create_read( *m_src_image_ctx, io::IMAGE_DISPATCH_LAYER_INTERNAL_START, aio_comp, @@ -274,6 +272,14 @@ void ObjectCopyRequest::handle_update_object_map(int r) { template void ObjectCopyRequest::process_copyup() { + if (m_snapshot_sparse_bufferlist.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; + } + ldout(m_cct, 20) << dendl; // let dispatch layers have a chance to process the data but @@ -654,14 +660,21 @@ void ObjectCopyRequest::compute_zero_ops() { } } - bool fast_diff = m_dst_image_ctx->test_features(RBD_FEATURE_FAST_DIFF); - uint64_t prev_end_size = 0; - // ensure we have a zeroed interval for each snapshot for (auto& [src_snap_seq, _] : m_snap_map) { - m_dst_zero_interval[src_snap_seq]; + if (m_src_snap_id_start < src_snap_seq) { + m_dst_zero_interval[src_snap_seq]; + } } + // exists if copying from an arbitrary snapshot w/o any deltas in the + // start snapshot slot (i.e. DNE) + bool object_exists = ( + m_src_snap_id_start > 0 && + m_snapshot_delta.count({m_src_snap_id_start, m_src_snap_id_start}) == 0); + bool fast_diff = m_dst_image_ctx->test_features(RBD_FEATURE_FAST_DIFF); + uint64_t prev_end_size = 0; + // compute zero ops from the zeroed intervals for (auto &it : m_dst_zero_interval) { auto src_snap_seq = it.first; @@ -679,11 +692,12 @@ 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_snapshot_sparse_bufferlist[src_snap_seq].insert( 0, m_dst_image_ctx->layout.object_size, {io::SPARSE_EXTENT_STATE_ZEROED, m_dst_image_ctx->layout.object_size}); + object_exists = false; prev_end_size = 0; continue; } @@ -707,21 +721,16 @@ 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); } } } - uint64_t end_size = prev_end_size; - // update end_size if there are writes into higher offsets + uint64_t end_size = prev_end_size; auto iter = m_snapshot_sparse_bufferlist.find(src_snap_seq); if (iter != m_snapshot_sparse_bufferlist.end()) { for (auto &sparse_bufferlist : iter->second) { + object_exists = true; end_size = std::max( end_size, sparse_bufferlist.get_off() + sparse_bufferlist.get_len()); } @@ -752,6 +761,8 @@ void ObjectCopyRequest::compute_zero_ops() { object_extent.offset, length, {io::SPARSE_EXTENT_STATE_ZEROED, length}); } + + object_exists = (object_extent.offset > 0 || hide_parent); end_size = std::min(end_size, object_extent.offset); } else { // zero interval inside the object @@ -761,19 +772,24 @@ void ObjectCopyRequest::compute_zero_ops() { m_snapshot_sparse_bufferlist[src_snap_seq].insert( object_extent.offset, object_extent.length, {io::SPARSE_EXTENT_STATE_ZEROED, object_extent.length}); + 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_snapshot_sparse_bufferlist.count(src_snap_seq) == 0) { - 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_snapshot_sparse_bufferlist.count(src_snap_seq) == 0) { + 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/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc index e5f9f74b448da..b6848995448c5 100644 --- a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc @@ -14,6 +14,7 @@ #include "librbd/api/Image.h" #include "librbd/api/Io.h" #include "librbd/deep_copy/ObjectCopyRequest.h" +#include "librbd/deep_copy/Utils.h" #include "librbd/io/ReadResult.h" #include "librbd/io/Utils.h" #include "test/librados_test_stub/MockTestMemIoCtxImpl.h" @@ -148,6 +149,7 @@ public: asio::ContextWQ *m_work_queue; SnapMap m_snap_map; + SnapSeqs m_snap_seqs; std::vector m_src_snap_ids; std::vector m_dst_snap_ids; @@ -227,12 +229,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, 0, nullptr, on_finish); + snap_map, 0, 0, nullptr, on_finish); } void expect_read(librbd::MockTestImageCtx& mock_image_ctx, @@ -374,6 +382,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); @@ -506,8 +515,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); InSequence seq; expect_list_snaps(mock_src_image_ctx, -ENOENT); @@ -537,8 +546,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_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -579,8 +588,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); InSequence seq; expect_list_snaps(mock_src_image_ctx, 0); @@ -612,8 +621,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_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -666,8 +675,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_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -730,8 +739,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_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -784,8 +793,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_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -837,8 +846,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); InSequence seq; expect_list_snaps(mock_src_image_ctx, 0); @@ -872,8 +881,8 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, PrepareCopyupError) { 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); InSequence seq; expect_list_snaps(mock_src_image_ctx, 0); @@ -924,8 +933,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) { interval_set four; scribble(m_src_image_ctx, 10, 102400, &four); - // src end's dst snap seqs should point to HEAD revision - m_snap_map[m_src_image_ctx->snaps[0]][0] = CEPH_NOSNAP; + // map should begin after src start and src end's dst snap seqs should + // point to HEAD revision + 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); @@ -944,6 +955,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); @@ -978,5 +990,75 @@ 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); + + expect_list_snaps(mock_src_image_ctx, 0); + + expect_read(mock_src_image_ctx, m_src_snap_ids[0], 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_ctx(get_mock_io_ctx( + request1->get_dst_io_ctx())); + expect_prepare_copyup(mock_dst_image_ctx); + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx, 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); + + expect_list_snaps(mock_src_image_ctx, 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