From: Ilya Dryomov Date: Sun, 26 Jun 2022 11:05:09 +0000 (+0200) Subject: librbd: update progress for non-existent objects on deep-copy X-Git-Tag: v18.0.0~605^2~1 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=6813a7879146aec40f204a174b40a5a54e00b780;p=ceph.git librbd: update progress for non-existent objects on deep-copy As a side effect of commit e5a21e904142 ("librbd: deep-copy image copy state machine skips clean objects"), handle_object_copy() stopped being called for non-existent objects. This broke progress_object_no logic, which expects to "see" all object numbers so that update_progress() callback invocations can be ordered. Currently update_progress() based progress reporting gets stuck after encountering a hole in the image. To fix, arrange for handle_object_copy() to be called for all object numbers, even if ObjectCopyRequest isn't created. Defer the extra call to the image work queue to avoid locking issues. Fixes: https://tracker.ceph.com/issues/56181 Signed-off-by: Ilya Dryomov --- diff --git a/src/librbd/deep_copy/ImageCopyRequest.cc b/src/librbd/deep_copy/ImageCopyRequest.cc index 2338dfde9d63b..e880071118730 100644 --- a/src/librbd/deep_copy/ImageCopyRequest.cc +++ b/src/librbd/deep_copy/ImageCopyRequest.cc @@ -5,6 +5,7 @@ #include "ObjectCopyRequest.h" #include "common/errno.h" #include "librbd/Utils.h" +#include "librbd/asio/ContextWQ.h" #include "librbd/deep_copy/Handler.h" #include "librbd/deep_copy/Utils.h" #include "librbd/object_map/DiffRequest.h" @@ -18,6 +19,7 @@ namespace librbd { namespace deep_copy { +using librbd::util::create_async_context_callback; using librbd::util::create_context_callback; using librbd::util::unique_lock_name; @@ -176,6 +178,13 @@ int ImageCopyRequest::send_next_object_copy() { } uint64_t ono = m_object_no++; + Context *ctx = new LambdaContext( + [this, ono](int r) { + handle_object_copy(ono, r); + }); + + ldout(m_cct, 20) << "object_num=" << ono << dendl; + ++m_current_ops; uint8_t object_diff_state = object_map::DIFF_STATE_HOLE; if (m_object_diff_state.size() > 0) { @@ -199,13 +208,11 @@ int ImageCopyRequest::send_next_object_copy() { if (object_diff_state == object_map::DIFF_STATE_HOLE) { ldout(m_cct, 20) << "skipping non-existent object " << ono << dendl; - return 1; + create_async_context_callback(*m_src_image_ctx, ctx)->complete(0); + return 0; } } - ldout(m_cct, 20) << "object_num=" << ono << dendl; - ++m_current_ops; - uint32_t flags = 0; if (m_flatten) { flags |= OBJECT_COPY_REQUEST_FLAG_FLATTEN; @@ -215,10 +222,6 @@ int ImageCopyRequest::send_next_object_copy() { flags |= OBJECT_COPY_REQUEST_FLAG_EXISTS_CLEAN; } - Context *ctx = new LambdaContext( - [this, ono](int r) { - handle_object_copy(ono, r); - }); auto req = ObjectCopyRequest::create( m_src_image_ctx, m_dst_image_ctx, m_src_snap_id_start, m_dst_snap_id_start, m_snap_map, ono, flags, m_handler, ctx); @@ -258,13 +261,7 @@ void ImageCopyRequest::handle_object_copy(uint64_t object_no, int r) { } } - while (true) { - r = send_next_object_copy(); - if (r != 1) { - break; - } - } - + send_next_object_copy(); complete = (m_current_ops == 0) && !m_updating_progress; } diff --git a/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc index 4bcca201f7a93..2763554b94667 100644 --- a/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc @@ -312,6 +312,7 @@ TEST_F(TestMockDeepCopyImageCopyRequest, FastDiffNonExistent) { expect_get_image_size(mock_src_image_ctx, 1 << m_src_image_ctx->order); expect_get_image_size(mock_src_image_ctx, 0); + expect_op_work_queue(mock_src_image_ctx); librbd::deep_copy::NoOpHandler no_op; C_SaferCond ctx; @@ -391,6 +392,85 @@ TEST_F(TestMockDeepCopyImageCopyRequest, FastDiffExistsClean) { ASSERT_EQ(0, ctx.wait()); } +TEST_F(TestMockDeepCopyImageCopyRequest, FastDiffMix) { + librados::snap_t snap_id_end; + ASSERT_EQ(0, create_snap("copy", &snap_id_end)); + + uint64_t object_count = 12; + + librbd::MockTestImageCtx mock_src_image_ctx(*m_src_image_ctx); + librbd::MockTestImageCtx mock_dst_image_ctx(*m_dst_image_ctx); + MockObjectCopyRequest mock_object_copy_request; + + InSequence seq; + + MockDiffRequest mock_diff_request; + BitVector<2> diff_state; + diff_state.resize(object_count); + diff_state[1] = object_map::DIFF_STATE_DATA_UPDATED; + diff_state[2] = object_map::DIFF_STATE_DATA_UPDATED; + diff_state[3] = object_map::DIFF_STATE_DATA; + diff_state[5] = object_map::DIFF_STATE_DATA_UPDATED; + diff_state[8] = object_map::DIFF_STATE_DATA; + diff_state[9] = object_map::DIFF_STATE_DATA; + diff_state[10] = object_map::DIFF_STATE_DATA_UPDATED; + expect_diff_send(mock_diff_request, diff_state, 0); + + 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); + + expect_op_work_queue(mock_src_image_ctx); + expect_object_copy_send(mock_object_copy_request, 0); + expect_object_copy_send(mock_object_copy_request, 0); + expect_object_copy_send(mock_object_copy_request, + OBJECT_COPY_REQUEST_FLAG_EXISTS_CLEAN); + expect_op_work_queue(mock_src_image_ctx); + expect_object_copy_send(mock_object_copy_request, 0); + expect_op_work_queue(mock_src_image_ctx); + expect_object_copy_send(mock_object_copy_request, + OBJECT_COPY_REQUEST_FLAG_EXISTS_CLEAN); + expect_object_copy_send(mock_object_copy_request, + OBJECT_COPY_REQUEST_FLAG_EXISTS_CLEAN); + expect_object_copy_send(mock_object_copy_request, 0); + expect_op_work_queue(mock_src_image_ctx); + + std::vector seen(object_count); + struct Handler : public librbd::deep_copy::NoOpHandler { + Handler(std::vector* seen) : m_seen(seen) {} + + int update_progress(uint64_t object_no, uint64_t end_object_no) override { + EXPECT_THAT(object_no, ::testing::AllOf(::testing::Ge(1), + ::testing::Le(m_seen->size()))); + EXPECT_EQ(end_object_no, m_seen->size()); + EXPECT_FALSE((*m_seen)[object_no - 1]); + (*m_seen)[object_no - 1] = true; + return 0; + } + + std::vector* m_seen; + } handler(&seen); + + 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, &handler, &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, 1, nullptr, 0)); + ASSERT_TRUE(complete_object_copy(mock_object_copy_request, 2, nullptr, 0)); + ASSERT_TRUE(complete_object_copy(mock_object_copy_request, 3, nullptr, 0)); + ASSERT_TRUE(complete_object_copy(mock_object_copy_request, 5, nullptr, 0)); + ASSERT_TRUE(complete_object_copy(mock_object_copy_request, 8, nullptr, 0)); + ASSERT_TRUE(complete_object_copy(mock_object_copy_request, 9, nullptr, 0)); + ASSERT_TRUE(complete_object_copy(mock_object_copy_request, 10, nullptr, 0)); + ASSERT_EQ(0, ctx.wait()); + + EXPECT_THAT(seen, ::testing::Each(::testing::IsTrue())); +} + TEST_F(TestMockDeepCopyImageCopyRequest, OutOfOrder) { std::string max_ops_str; ASSERT_EQ(0, _rados.conf_get("rbd_concurrent_management_ops", max_ops_str));