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 <idryomov@gmail.com>

diff --git a/src/librbd/deep_copy/ImageCopyRequest.cc b/src/librbd/deep_copy/ImageCopyRequest.cc
index 2338dfde9d63bea340b6d096b423f619c30fa47c..e880071118730422f7f2371f5c2f266e974fcee7 100644 (file)
--- 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<I>::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<I>::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<I>::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<I>::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<I>::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 4bcca201f7a93e154182b0fd01f2d27dfcc9bc95..2763554b94667aafd7f8a7d4eefefad063208975 100644 (file)
--- 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<bool> seen(object_count);
+  struct Handler : public librbd::deep_copy::NoOpHandler {
+    Handler(std::vector<bool>* 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<bool>* 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));