From 20073d8da7b65b34041117b8f7b1e6f975a6d1d2 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Fri, 29 Jan 2021 10:44:38 -0500 Subject: [PATCH] librbd/deep_copy: skip snap list if object is known to be clean If the fast-diff indicates that the destination object should exist and that it hasn't changed, there shouldn't be a need to issue the snap list operation. Instead, just update the destination object map to indicate the existence of the object. Signed-off-by: Jason Dillaman (cherry picked from commit 235b27a8f08a82fc46efa8b6a3e5cb8a5276956b) --- src/librbd/deep_copy/ImageCopyRequest.cc | 25 ++++-- src/librbd/deep_copy/ObjectCopyRequest.cc | 13 ++- src/librbd/deep_copy/Types.h | 5 +- .../deep_copy/test_mock_ImageCopyRequest.cc | 87 +++++++++++++++++-- .../deep_copy/test_mock_ObjectCopyRequest.cc | 67 +++++++++++--- 5 files changed, 164 insertions(+), 33 deletions(-) diff --git a/src/librbd/deep_copy/ImageCopyRequest.cc b/src/librbd/deep_copy/ImageCopyRequest.cc index 4adb836a65b5d..2338dfde9d63b 100644 --- a/src/librbd/deep_copy/ImageCopyRequest.cc +++ b/src/librbd/deep_copy/ImageCopyRequest.cc @@ -177,21 +177,28 @@ int ImageCopyRequest::send_next_object_copy() { uint64_t ono = m_object_no++; + uint8_t object_diff_state = object_map::DIFF_STATE_HOLE; if (m_object_diff_state.size() > 0) { std::set src_objects; map_src_objects(ono, &src_objects); - bool skip = true; for (auto src_ono : src_objects) { - if (src_ono >= m_object_diff_state.size() || - m_object_diff_state[src_ono] != object_map::DIFF_STATE_HOLE) { - skip = false; - break; + if (src_ono >= m_object_diff_state.size()) { + object_diff_state = object_map::DIFF_STATE_DATA_UPDATED; + } else { + auto state = m_object_diff_state[src_ono]; + if ((state == object_map::DIFF_STATE_HOLE_UPDATED && + object_diff_state != object_map::DIFF_STATE_DATA_UPDATED) || + (state == object_map::DIFF_STATE_DATA && + object_diff_state == object_map::DIFF_STATE_HOLE) || + (state == object_map::DIFF_STATE_DATA_UPDATED)) { + object_diff_state = state; + } } } - if (skip) { - ldout(m_cct, 20) << "skipping clean object " << ono << dendl; + if (object_diff_state == object_map::DIFF_STATE_HOLE) { + ldout(m_cct, 20) << "skipping non-existent object " << ono << dendl; return 1; } } @@ -203,6 +210,10 @@ int ImageCopyRequest::send_next_object_copy() { if (m_flatten) { flags |= OBJECT_COPY_REQUEST_FLAG_FLATTEN; } + if (object_diff_state == object_map::DIFF_STATE_DATA) { + // no source objects have been updated and at least one has clean data + flags |= OBJECT_COPY_REQUEST_FLAG_EXISTS_CLEAN; + } Context *ctx = new LambdaContext( [this, ono](int r) { diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc index d114a2457e007..235f77183d5ce 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.cc +++ b/src/librbd/deep_copy/ObjectCopyRequest.cc @@ -77,6 +77,16 @@ void ObjectCopyRequest::send_list_snaps() { m_dst_image_ctx->layout.object_size, m_image_extents); ldout(m_cct, 20) << "image_extents=" << m_image_extents << dendl; + auto ctx = create_async_context_callback( + *m_src_image_ctx, create_context_callback< + ObjectCopyRequest, &ObjectCopyRequest::handle_list_snaps>(this)); + if ((m_flags & OBJECT_COPY_REQUEST_FLAG_EXISTS_CLEAN) != 0) { + // skip listing the snaps if we know the destination exists and is clean, + // but we do need to update the object-map + ctx->complete(0); + return; + } + io::SnapIds snap_ids; snap_ids.reserve(1 + m_snap_map.size()); snap_ids.push_back(m_src_snap_id_start); @@ -90,9 +100,6 @@ void ObjectCopyRequest::send_list_snaps() { m_snapshot_delta.clear(); - auto ctx = create_async_context_callback( - *m_src_image_ctx, create_context_callback< - ObjectCopyRequest, &ObjectCopyRequest::handle_list_snaps>(this)); auto aio_comp = io::AioCompletion::create_and_start( ctx, get_image_ctx(m_src_image_ctx), io::AIO_TYPE_GENERIC); auto req = io::ImageDispatchSpec::create_list_snaps( diff --git a/src/librbd/deep_copy/Types.h b/src/librbd/deep_copy/Types.h index e6aa980494b12..9cd4835b3221b 100644 --- a/src/librbd/deep_copy/Types.h +++ b/src/librbd/deep_copy/Types.h @@ -12,8 +12,9 @@ namespace librbd { namespace deep_copy { enum { - OBJECT_COPY_REQUEST_FLAG_FLATTEN = 1U << 0, - OBJECT_COPY_REQUEST_FLAG_MIGRATION = 1U << 1, + OBJECT_COPY_REQUEST_FLAG_FLATTEN = 1U << 0, + OBJECT_COPY_REQUEST_FLAG_MIGRATION = 1U << 1, + OBJECT_COPY_REQUEST_FLAG_EXISTS_CLEAN = 1U << 2, }; typedef std::vector SnapIds; diff --git a/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc index bcb2daed8a0b2..4fac9ea41dc74 100644 --- a/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc @@ -59,6 +59,7 @@ struct ObjectCopyRequest { ceph_assert(s_instance != nullptr); std::lock_guard locker{s_instance->lock}; s_instance->snap_map = &snap_map; + s_instance->flags = flags; s_instance->object_contexts[object_number] = on_finish; s_instance->cond.notify_all(); return s_instance; @@ -71,6 +72,7 @@ struct ObjectCopyRequest { const SnapMap *snap_map = nullptr; std::map object_contexts; + uint32_t flags = 0; ObjectCopyRequest() { s_instance = this; @@ -169,8 +171,12 @@ public: })); } - void expect_object_copy_send(MockObjectCopyRequest &mock_object_copy_request) { - EXPECT_CALL(mock_object_copy_request, send()); + void expect_object_copy_send(MockObjectCopyRequest &mock_object_copy_request, + uint32_t flags) { + EXPECT_CALL(mock_object_copy_request, send()) + .WillOnce(Invoke([&mock_object_copy_request, flags]() { + ASSERT_EQ(flags, mock_object_copy_request.flags); + })); } bool complete_object_copy(MockObjectCopyRequest &mock_object_copy_request, @@ -273,7 +279,7 @@ TEST_F(TestMockDeepCopyImageCopyRequest, SimpleImage) { expect_diff_send(mock_diff_request, {}, -EINVAL); expect_get_image_size(mock_src_image_ctx, 1 << m_src_image_ctx->order); expect_get_image_size(mock_src_image_ctx, 0); - expect_object_copy_send(mock_object_copy_request); + expect_object_copy_send(mock_object_copy_request, 0); librbd::deep_copy::NoOpHandler no_op; C_SaferCond ctx; @@ -288,7 +294,7 @@ TEST_F(TestMockDeepCopyImageCopyRequest, SimpleImage) { ASSERT_EQ(0, ctx.wait()); } -TEST_F(TestMockDeepCopyImageCopyRequest, FastDiff) { +TEST_F(TestMockDeepCopyImageCopyRequest, FastDiffNonExistent) { librados::snap_t snap_id_end; ASSERT_EQ(0, create_snap("copy", &snap_id_end)); @@ -316,6 +322,73 @@ TEST_F(TestMockDeepCopyImageCopyRequest, FastDiff) { ASSERT_EQ(0, ctx.wait()); } +TEST_F(TestMockDeepCopyImageCopyRequest, FastDiffExistsDirty) { + librados::snap_t snap_id_end; + ASSERT_EQ(0, create_snap("copy", &snap_id_end)); + + librbd::MockTestImageCtx mock_src_image_ctx(*m_src_image_ctx); + librbd::MockTestImageCtx mock_dst_image_ctx(*m_dst_image_ctx); + + InSequence seq; + + MockDiffRequest mock_diff_request; + BitVector<2> diff_state; + diff_state.resize(1); + diff_state[0] = object_map::DIFF_STATE_DATA_UPDATED; + expect_diff_send(mock_diff_request, diff_state, 0); + + expect_get_image_size(mock_src_image_ctx, 1 << m_src_image_ctx->order); + expect_get_image_size(mock_src_image_ctx, 0); + MockObjectCopyRequest mock_object_copy_request; + expect_object_copy_send(mock_object_copy_request, 0); + + librbd::deep_copy::NoOpHandler no_op; + C_SaferCond ctx; + auto request = new MockImageCopyRequest(&mock_src_image_ctx, + &mock_dst_image_ctx, + 0, snap_id_end, 0, false, boost::none, + m_snap_seqs, &no_op, &ctx); + request->send(); + + ASSERT_EQ(m_snap_map, wait_for_snap_map(mock_object_copy_request)); + ASSERT_TRUE(complete_object_copy(mock_object_copy_request, 0, nullptr, 0)); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockDeepCopyImageCopyRequest, FastDiffExistsClean) { + librados::snap_t snap_id_end; + ASSERT_EQ(0, create_snap("copy", &snap_id_end)); + + librbd::MockTestImageCtx mock_src_image_ctx(*m_src_image_ctx); + librbd::MockTestImageCtx mock_dst_image_ctx(*m_dst_image_ctx); + + InSequence seq; + + MockDiffRequest mock_diff_request; + BitVector<2> diff_state; + diff_state.resize(1); + diff_state[0] = object_map::DIFF_STATE_DATA; + expect_diff_send(mock_diff_request, diff_state, 0); + + expect_get_image_size(mock_src_image_ctx, 1 << m_src_image_ctx->order); + expect_get_image_size(mock_src_image_ctx, 0); + MockObjectCopyRequest mock_object_copy_request; + expect_object_copy_send(mock_object_copy_request, + OBJECT_COPY_REQUEST_FLAG_EXISTS_CLEAN); + + librbd::deep_copy::NoOpHandler no_op; + C_SaferCond ctx; + auto request = new MockImageCopyRequest(&mock_src_image_ctx, + &mock_dst_image_ctx, + 0, snap_id_end, 0, false, boost::none, + m_snap_seqs, &no_op, &ctx); + request->send(); + + ASSERT_EQ(m_snap_map, wait_for_snap_map(mock_object_copy_request)); + ASSERT_TRUE(complete_object_copy(mock_object_copy_request, 0, nullptr, 0)); + ASSERT_EQ(0, ctx.wait()); +} + TEST_F(TestMockDeepCopyImageCopyRequest, OutOfOrder) { std::string max_ops_str; ASSERT_EQ(0, _rados.conf_get("rbd_concurrent_management_ops", max_ops_str)); @@ -409,7 +482,7 @@ TEST_F(TestMockDeepCopyImageCopyRequest, SnapshotSubset) { expect_get_image_size(mock_src_image_ctx, 0); expect_get_image_size(mock_src_image_ctx, 0); expect_get_image_size(mock_src_image_ctx, 0); - expect_object_copy_send(mock_object_copy_request); + expect_object_copy_send(mock_object_copy_request, 0); librbd::deep_copy::NoOpHandler no_op; C_SaferCond ctx; @@ -441,7 +514,7 @@ TEST_F(TestMockDeepCopyImageCopyRequest, RestartPartialSync) { expect_diff_send(mock_diff_request, {}, -EINVAL); expect_get_image_size(mock_src_image_ctx, 2 * (1 << m_src_image_ctx->order)); expect_get_image_size(mock_src_image_ctx, 0); - expect_object_copy_send(mock_object_copy_request); + expect_object_copy_send(mock_object_copy_request, 0); librbd::deep_copy::NoOpHandler no_op; C_SaferCond ctx; @@ -477,7 +550,7 @@ TEST_F(TestMockDeepCopyImageCopyRequest, Cancel) { expect_diff_send(mock_diff_request, {}, -EINVAL); expect_get_image_size(mock_src_image_ctx, 1 << m_src_image_ctx->order); expect_get_image_size(mock_src_image_ctx, 0); - expect_object_copy_send(mock_object_copy_request); + expect_object_copy_send(mock_object_copy_request, 0); librbd::deep_copy::NoOpHandler no_op; C_SaferCond ctx; diff --git a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc index b6848995448c5..d8bb0678a1355 100644 --- a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc @@ -231,7 +231,7 @@ public: librados::snap_t src_snap_id_start, librados::snap_t src_snap_id_end, librados::snap_t dst_snap_id_start, - Context *on_finish) { + uint32_t flags, 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, @@ -240,7 +240,7 @@ public: 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, - snap_map, 0, 0, nullptr, on_finish); + snap_map, 0, flags, nullptr, on_finish); } void expect_read(librbd::MockTestImageCtx& mock_image_ctx, @@ -516,7 +516,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, DNE) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, 0, - CEPH_NOSNAP, 0, &ctx); + CEPH_NOSNAP, 0, 0, &ctx); InSequence seq; expect_list_snaps(mock_src_image_ctx, -ENOENT); @@ -547,7 +547,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Write) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, 0, - CEPH_NOSNAP, 0, &ctx); + CEPH_NOSNAP, 0, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -589,7 +589,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadError) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, 0, - CEPH_NOSNAP, 0, &ctx); + CEPH_NOSNAP, 0, 0, &ctx); InSequence seq; expect_list_snaps(mock_src_image_ctx, 0); @@ -622,7 +622,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteError) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, 0, - CEPH_NOSNAP, 0, &ctx); + CEPH_NOSNAP, 0, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -676,7 +676,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, 0, - CEPH_NOSNAP, 0, &ctx); + CEPH_NOSNAP, 0, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -740,7 +740,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Trim) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, 0, - CEPH_NOSNAP, 0, &ctx); + CEPH_NOSNAP, 0, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -794,7 +794,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Remove) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, 0, - CEPH_NOSNAP, 0, &ctx); + CEPH_NOSNAP, 0, 0, &ctx); librados::MockTestMemIoCtxImpl &mock_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -847,7 +847,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ObjectMapUpdateError) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, 0, - CEPH_NOSNAP, 0, &ctx); + CEPH_NOSNAP, 0, 0, &ctx); InSequence seq; expect_list_snaps(mock_src_image_ctx, 0); @@ -882,7 +882,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, PrepareCopyupError) { C_SaferCond ctx; MockObjectCopyRequest *request = create_request(mock_src_image_ctx, mock_dst_image_ctx, 0, - CEPH_NOSNAP, 0, &ctx); + CEPH_NOSNAP, 0, 0, &ctx); InSequence seq; expect_list_snaps(mock_src_image_ctx, 0); @@ -957,7 +957,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) { src_snap_id_start, CEPH_NOSNAP, dst_snap_id_start, - &ctx); + 0, &ctx); librados::MockTestMemIoCtxImpl &mock_dst_io_ctx(get_mock_io_ctx( request->get_dst_io_ctx())); @@ -1014,7 +1014,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Incremental) { C_SaferCond ctx1; auto request1 = create_request(mock_src_image_ctx, mock_dst_image_ctx, - 0, m_src_snap_ids[0], 0, &ctx1); + 0, m_src_snap_ids[0], 0, 0, &ctx1); expect_list_snaps(mock_src_image_ctx, 0); @@ -1041,7 +1041,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Incremental) { 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); + m_dst_snap_ids[0], 0, &ctx2); expect_list_snaps(mock_src_image_ctx, 0); expect_start_op(mock_exclusive_lock); @@ -1060,5 +1060,44 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Incremental) { ASSERT_EQ(0, compare_objects()); } +TEST_F(TestMockDeepCopyObjectCopyRequest, SkipSnapList) { + 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); + + ASSERT_EQ(0, create_snap("snap1")); + mock_dst_image_ctx.snaps = m_dst_image_ctx->snaps; + + InSequence seq; + + // clean (no-updates) snapshots + ASSERT_EQ(0, create_snap("snap2")); + mock_dst_image_ctx.snaps = m_dst_image_ctx->snaps; + + C_SaferCond ctx; + auto request = create_request(mock_src_image_ctx, mock_dst_image_ctx, + m_src_snap_ids[0], m_src_snap_ids[1], + m_dst_snap_ids[0], + OBJECT_COPY_REQUEST_FLAG_EXISTS_CLEAN, &ctx); + + 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); + + request->send(); + ASSERT_EQ(0, ctx.wait()); +} + } // namespace deep_copy } // namespace librbd -- 2.39.5