From e5a21e904142f8d728ba5de1eecf940b6b8329b5 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 10 Mar 2020 17:04:59 -0400 Subject: [PATCH] librbd: deep-copy image copy state machine skips clean objects If fast-diff is enabled (and valid), the image-copy state machine will now attempt to skip any objects that are flagged as clean (DIFF_STATE_NONE). For larger images, this should avoid the need to iterate over potentially tens or hundreds of thousands of objects. Fixes: https://tracker.ceph.com/issues/43590 Signed-off-by: Jason Dillaman --- src/librbd/deep_copy/ImageCopyRequest.cc | 64 +++++++++++--- src/librbd/deep_copy/ImageCopyRequest.h | 12 ++- .../deep_copy/test_mock_ImageCopyRequest.cc | 83 ++++++++++++++++++- 3 files changed, 146 insertions(+), 13 deletions(-) diff --git a/src/librbd/deep_copy/ImageCopyRequest.cc b/src/librbd/deep_copy/ImageCopyRequest.cc index 64be5fee90815..09d51169f1a3b 100644 --- a/src/librbd/deep_copy/ImageCopyRequest.cc +++ b/src/librbd/deep_copy/ImageCopyRequest.cc @@ -8,6 +8,7 @@ #include "librbd/deep_copy/Utils.h" #include "librbd/image/CloseRequest.h" #include "librbd/image/OpenRequest.h" +#include "librbd/object_map/DiffRequest.h" #include "osdc/Striper.h" #define dout_subsys ceph_subsys_rbd @@ -53,7 +54,7 @@ void ImageCopyRequest::send() { return; } - send_object_copies(); + compute_diff(); } template @@ -64,6 +65,30 @@ void ImageCopyRequest::cancel() { m_canceled = true; } +template +void ImageCopyRequest::compute_diff() { + ldout(m_cct, 10) << dendl; + + auto ctx = create_context_callback< + ImageCopyRequest, &ImageCopyRequest::handle_compute_diff>(this); + auto req = object_map::DiffRequest::create(m_src_image_ctx, m_src_snap_id_start, + m_src_snap_id_end, &m_object_diff_state, + ctx); + req->send(); +} + +template +void ImageCopyRequest::handle_compute_diff(int r) { + ldout(m_cct, 10) << "r=" << r << dendl; + + if (r < 0) { + ldout(m_cct, 10) << "fast-diff optimization disabled" << dendl; + m_object_diff_state.resize(0); + } + + send_object_copies(); +} + template void ImageCopyRequest::send_object_copies() { m_object_no = 0; @@ -87,14 +112,18 @@ void ImageCopyRequest::send_object_copies() { bool complete; { std::lock_guard locker{m_lock}; - for (uint64_t i = 0; - i < m_src_image_ctx->config.template get_val("rbd_concurrent_management_ops"); - ++i) { - send_next_object_copy(); - if (m_ret_val < 0 && m_current_ops == 0) { + auto max_ops = m_src_image_ctx->config.template get_val( + "rbd_concurrent_management_ops"); + + // attempt to schedule at least 'max_ops' initial requests where + // some objects might be skipped if fast-diff notes no change + while (m_current_ops < max_ops) { + int r = send_next_object_copy(); + if (r < 0) { break; } } + complete = (m_current_ops == 0) && !m_updating_progress; } @@ -104,7 +133,7 @@ void ImageCopyRequest::send_object_copies() { } template -void ImageCopyRequest::send_next_object_copy() { +int ImageCopyRequest::send_next_object_copy() { ceph_assert(ceph_mutex_is_locked(m_lock)); if (m_canceled && m_ret_val == 0) { @@ -112,14 +141,20 @@ void ImageCopyRequest::send_next_object_copy() { m_ret_val = -ECANCELED; } - if (m_ret_val < 0 || m_object_no >= m_end_object_no) { - return; + if (m_ret_val < 0) { + return m_ret_val; + } else if (m_object_no >= m_end_object_no) { + return -ENODATA; } uint64_t ono = m_object_no++; + if (ono < m_object_diff_state.size() && + m_object_diff_state[ono] == object_map::DIFF_STATE_NONE) { + ldout(m_cct, 20) << "skipping clean object " << ono << dendl; + return 1; + } ldout(m_cct, 20) << "object_num=" << ono << dendl; - ++m_current_ops; Context *ctx = new LambdaContext( @@ -130,6 +165,7 @@ void ImageCopyRequest::send_next_object_copy() { m_src_image_ctx, m_dst_image_ctx, m_src_snap_id_start, m_dst_snap_id_start, m_snap_map, ono, m_flatten, ctx); req->send(); + return 0; } template @@ -164,7 +200,13 @@ void ImageCopyRequest::handle_object_copy(uint64_t object_no, int r) { } } - send_next_object_copy(); + while (true) { + r = send_next_object_copy(); + if (r != 1) { + break; + } + } + complete = (m_current_ops == 0) && !m_updating_progress; } diff --git a/src/librbd/deep_copy/ImageCopyRequest.h b/src/librbd/deep_copy/ImageCopyRequest.h index 903952254427a..45967fdaf2a54 100644 --- a/src/librbd/deep_copy/ImageCopyRequest.h +++ b/src/librbd/deep_copy/ImageCopyRequest.h @@ -6,6 +6,7 @@ #include "include/int_types.h" #include "include/rados/librados.hpp" +#include "common/bit_vector.hpp" #include "common/ceph_mutex.h" #include "common/RefCountedObj.h" #include "librbd/Types.h" @@ -59,6 +60,10 @@ private: * @verbatim * * + * | + * v + * COMPUTE_DIFF + * | * | . . . . . * | . . (parallel execution of * v v . multiple objects at once) @@ -94,8 +99,13 @@ private: SnapMap m_snap_map; int m_ret_val = 0; + BitVector<2> m_object_diff_state; + + void compute_diff(); + void handle_compute_diff(int r); + void send_object_copies(); - void send_next_object_copy(); + int send_next_object_copy(); void handle_object_copy(uint64_t object_no, int r); void finish(int r); diff --git a/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc index 8becf673b0782..a0a0c18b47b60 100644 --- a/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc @@ -5,12 +5,13 @@ #include "include/rbd/librbd.hpp" #include "librbd/ImageCtx.h" #include "librbd/ImageState.h" +#include "librbd/internal.h" #include "librbd/Operations.h" #include "librbd/deep_copy/ImageCopyRequest.h" #include "librbd/deep_copy/ObjectCopyRequest.h" #include "librbd/image/CloseRequest.h" #include "librbd/image/OpenRequest.h" -#include "librbd/internal.h" +#include "librbd/object_map/DiffRequest.h" #include "test/librados_test_stub/MockTestMemIoCtxImpl.h" #include "test/librbd/mock/MockImageCtx.h" #include "test/librbd/test_support.h" @@ -122,6 +123,33 @@ OpenRequest* OpenRequest::s_instance = nullp } // namespace image +namespace object_map { + +template <> +struct DiffRequest { + BitVector<2>* object_diff_state = nullptr; + Context* on_finish = nullptr; + static DiffRequest* s_instance; + static DiffRequest* create(MockTestImageCtx *image_ctx, + uint64_t snap_id_start, uint64_t snap_id_end, + BitVector<2>* object_diff_state, + Context* on_finish) { + ceph_assert(s_instance != nullptr); + s_instance->object_diff_state = object_diff_state; + s_instance->on_finish = on_finish; + return s_instance; + } + + DiffRequest() { + s_instance = this; + } + + MOCK_METHOD0(send, void()); +}; + +DiffRequest* DiffRequest::s_instance = nullptr; + +} // namespace object_map } // namespace librbd // template definitions @@ -133,12 +161,14 @@ namespace deep_copy { using ::testing::_; using ::testing::InSequence; +using ::testing::Invoke; using ::testing::Return; class TestMockDeepCopyImageCopyRequest : public TestMockFixture { public: typedef ImageCopyRequest MockImageCopyRequest; typedef ObjectCopyRequest MockObjectCopyRequest; + typedef object_map::DiffRequest MockDiffRequest; librbd::ImageCtx *m_src_image_ctx; librbd::ImageCtx *m_dst_image_ctx; @@ -167,6 +197,17 @@ public: .WillOnce(Return(size)).RetiresOnSaturation(); } + void expect_diff_send(MockDiffRequest& mock_request, + const BitVector<2>& diff_state, int r) { + EXPECT_CALL(mock_request, send()) + .WillOnce(Invoke([this, &mock_request, diff_state, r]() { + if (r >= 0) { + *mock_request.object_diff_state = diff_state; + } + m_work_queue->queue(mock_request.on_finish, r); + })); + } + void expect_object_copy_send(MockObjectCopyRequest &mock_object_copy_request) { EXPECT_CALL(mock_object_copy_request, send()); } @@ -266,6 +307,8 @@ TEST_F(TestMockDeepCopyImageCopyRequest, SimpleImage) { MockObjectCopyRequest mock_object_copy_request; InSequence seq; + MockDiffRequest mock_diff_request; + 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); @@ -283,6 +326,34 @@ TEST_F(TestMockDeepCopyImageCopyRequest, SimpleImage) { ASSERT_EQ(0, ctx.wait()); } +TEST_F(TestMockDeepCopyImageCopyRequest, FastDiff) { + 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); + 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); + + librbd::NoOpProgressContext 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(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)); @@ -301,6 +372,8 @@ TEST_F(TestMockDeepCopyImageCopyRequest, OutOfOrder) { librbd::MockTestImageCtx mock_dst_image_ctx(*m_dst_image_ctx); MockObjectCopyRequest mock_object_copy_request; + MockDiffRequest mock_diff_request; + expect_diff_send(mock_diff_request, {}, -EINVAL); expect_get_image_size(mock_src_image_ctx, object_count * (1 << m_src_image_ctx->order)); expect_get_image_size(mock_src_image_ctx, 0); @@ -368,6 +441,8 @@ TEST_F(TestMockDeepCopyImageCopyRequest, SnapshotSubset) { MockObjectCopyRequest mock_object_copy_request; InSequence seq; + MockDiffRequest mock_diff_request; + 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_get_image_size(mock_src_image_ctx, 0); @@ -400,6 +475,8 @@ TEST_F(TestMockDeepCopyImageCopyRequest, RestartPartialSync) { MockObjectCopyRequest mock_object_copy_request; InSequence seq; + MockDiffRequest mock_diff_request; + 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); @@ -434,6 +511,8 @@ TEST_F(TestMockDeepCopyImageCopyRequest, Cancel) { MockObjectCopyRequest mock_object_copy_request; InSequence seq; + MockDiffRequest mock_diff_request; + 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); @@ -470,6 +549,8 @@ TEST_F(TestMockDeepCopyImageCopyRequest, Cancel_Inflight_Sync) { MockObjectCopyRequest mock_object_copy_request; InSequence seq; + MockDiffRequest mock_diff_request; + expect_diff_send(mock_diff_request, {}, -EINVAL); expect_get_image_size(mock_src_image_ctx, 6 * (1 << m_src_image_ctx->order)); expect_get_image_size(mock_src_image_ctx, m_image_size); -- 2.47.3