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 <dillaman@redhat.com>
(cherry picked from commit 235b27a8f08a82fc46efa8b6a3e5cb8a5276956b)

diff --git a/src/librbd/deep_copy/ImageCopyRequest.cc b/src/librbd/deep_copy/ImageCopyRequest.cc
index 4adb836a65b5d50c94c5f000aa1299e3beb53c9c..2338dfde9d63bea340b6d096b423f619c30fa47c 100644 (file)
--- a/src/librbd/deep_copy/ImageCopyRequest.cc
+++ b/src/librbd/deep_copy/ImageCopyRequest.cc
@@ -177,21 +177,28 @@ int ImageCopyRequest<I>::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<uint64_t> 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<I>::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 d114a2457e0076482babd954052186b7ce6eebab..235f77183d5cece35c7e39e83ec2ff04d375f8df 100644 (file)
--- a/src/librbd/deep_copy/ObjectCopyRequest.cc
+++ b/src/librbd/deep_copy/ObjectCopyRequest.cc
@@ -77,6 +77,16 @@ void ObjectCopyRequest<I>::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<I>::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<I>::send_list_snaps() {
 
   m_snapshot_delta.clear();
 
-  auto ctx = create_async_context_callback(
-    *m_src_image_ctx, create_context_callback<
-      ObjectCopyRequest, &ObjectCopyRequest<I>::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 e6aa980494b12a37236c34e1db2c7fe55a7f277a..9cd4835b3221baae144890dd31b384a19dac5f7c 100644 (file)
--- 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<librados::snap_t> SnapIds;

diff --git a/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc
index bcb2daed8a0b2ecd757b63f57ada8bb3300a7a69..4fac9ea41dc7408f1ffe31fe5cc04a14ae968ebc 100644 (file)
--- 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<librbd::MockTestImageCtx> {
     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<librbd::MockTestImageCtx> {
 
   const SnapMap *snap_map = nullptr;
   std::map<uint64_t, Context *> 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 b6848995448c501571bc475964b4933231761040..d8bb0678a1355b6cbb2fbc85a73de9fc01d25184 100644 (file)
--- 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