librbd: deep-copy should update object-map before writing to object

For the original use-case of RBD mirroring it was (maybe) more
acceptable to write to the object before updating the object map
because an interrupted sync will be retried. However, when using
the deep-copy object copy state machine as part of copyup, it's
more likely that the object-map has the potential to become
out-of-sync with reality if it's updated after the object is
written.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc
index 569bb7e75a8bc5b983466e55a9010d9bd7d9bfcd..e423ccab1bace73b69ee997090edf6f982095177 100644 (file)
--- a/src/librbd/deep_copy/ObjectCopyRequest.cc
+++ b/src/librbd/deep_copy/ObjectCopyRequest.cc
@@ -124,7 +124,7 @@ void ObjectCopyRequest<I>::send_read() {
       return;
     }
 
-    send_write_object();
+    send_update_object_map();
     return;
   }
 
@@ -190,6 +190,83 @@ void ObjectCopyRequest<I>::handle_read(int r) {
   send_read();
 }
 
+template <typename I>
+void ObjectCopyRequest<I>::send_update_object_map() {
+  if (!m_dst_image_ctx->test_features(RBD_FEATURE_OBJECT_MAP) ||
+      m_dst_object_state.empty()) {
+    send_write_object();
+    return;
+  }
+
+  m_dst_image_ctx->owner_lock.lock_shared();
+  m_dst_image_ctx->image_lock.lock_shared();
+  if (m_dst_image_ctx->object_map == nullptr) {
+    // possible that exclusive lock was lost in background
+    lderr(m_cct) << "object map is not initialized" << dendl;
+
+    m_dst_image_ctx->image_lock.unlock_shared();
+    m_dst_image_ctx->owner_lock.unlock_shared();
+    finish(-EINVAL);
+    return;
+  }
+
+  auto &dst_object_state = *m_dst_object_state.begin();
+  auto it = m_snap_map.find(dst_object_state.first);
+  ceph_assert(it != m_snap_map.end());
+  auto dst_snap_id = it->second.front();
+  auto object_state = dst_object_state.second;
+  m_dst_object_state.erase(m_dst_object_state.begin());
+
+  ldout(m_cct, 20) << "dst_snap_id=" << dst_snap_id << ", object_state="
+                   << static_cast<uint32_t>(object_state) << dendl;
+
+  int r;
+  auto finish_op_ctx = start_lock_op(m_dst_image_ctx->owner_lock, &r);
+  if (finish_op_ctx == nullptr) {
+    lderr(m_cct) << "lost exclusive lock" << dendl;
+    m_dst_image_ctx->image_lock.unlock_shared();
+    m_dst_image_ctx->owner_lock.unlock_shared();
+    finish(r);
+    return;
+  }
+
+  auto ctx = new LambdaContext([this, finish_op_ctx](int r) {
+      handle_update_object_map(r);
+      finish_op_ctx->complete(0);
+    });
+
+  auto dst_image_ctx = m_dst_image_ctx;
+  bool sent = dst_image_ctx->object_map->template aio_update<
+    Context, &Context::complete>(dst_snap_id, m_dst_object_number, object_state,
+                                 {}, {}, false, ctx);
+
+  // NOTE: state machine might complete before we reach here
+  dst_image_ctx->image_lock.unlock_shared();
+  dst_image_ctx->owner_lock.unlock_shared();
+  if (!sent) {
+    ceph_assert(dst_snap_id == CEPH_NOSNAP);
+    ctx->complete(0);
+  }
+}
+
+template <typename I>
+void ObjectCopyRequest<I>::handle_update_object_map(int r) {
+  ldout(m_cct, 20) << "r=" << r << dendl;
+
+  if (r < 0) {
+    lderr(m_cct) << "failed to update object map: " << cpp_strerror(r) << dendl;
+    finish(r);
+    return;
+  }
+
+  if (!m_dst_object_state.empty()) {
+    send_update_object_map();
+    return;
+  }
+
+  send_write_object();
+}
+
 template <typename I>
 void ObjectCopyRequest<I>::send_write_object() {
   ceph_assert(!m_write_ops.empty());
@@ -317,82 +394,6 @@ void ObjectCopyRequest<I>::handle_write_object(int r) {
     return;
   }
 
-  send_update_object_map();
-}
-
-template <typename I>
-void ObjectCopyRequest<I>::send_update_object_map() {
-  if (!m_dst_image_ctx->test_features(RBD_FEATURE_OBJECT_MAP) ||
-      m_dst_object_state.empty()) {
-    finish(0);
-    return;
-  }
-
-  m_dst_image_ctx->owner_lock.lock_shared();
-  m_dst_image_ctx->image_lock.lock_shared();
-  if (m_dst_image_ctx->object_map == nullptr) {
-    // possible that exclusive lock was lost in background
-    lderr(m_cct) << "object map is not initialized" << dendl;
-
-    m_dst_image_ctx->image_lock.unlock_shared();
-    m_dst_image_ctx->owner_lock.unlock_shared();
-    finish(-EINVAL);
-    return;
-  }
-
-  auto &dst_object_state = *m_dst_object_state.begin();
-  auto it = m_snap_map.find(dst_object_state.first);
-  ceph_assert(it != m_snap_map.end());
-  auto dst_snap_id = it->second.front();
-  auto object_state = dst_object_state.second;
-  m_dst_object_state.erase(m_dst_object_state.begin());
-
-  ldout(m_cct, 20) << "dst_snap_id=" << dst_snap_id << ", object_state="
-                   << static_cast<uint32_t>(object_state) << dendl;
-
-  int r;
-  auto finish_op_ctx = start_lock_op(m_dst_image_ctx->owner_lock, &r);
-  if (finish_op_ctx == nullptr) {
-    lderr(m_cct) << "lost exclusive lock" << dendl;
-    m_dst_image_ctx->image_lock.unlock_shared();
-    m_dst_image_ctx->owner_lock.unlock_shared();
-    finish(r);
-    return;
-  }
-
-  auto ctx = new LambdaContext([this, finish_op_ctx](int r) {
-      handle_update_object_map(r);
-      finish_op_ctx->complete(0);
-    });
-
-  auto dst_image_ctx = m_dst_image_ctx;
-  bool sent = dst_image_ctx->object_map->template aio_update<
-    Context, &Context::complete>(dst_snap_id, m_dst_object_number, object_state,
-                                 {}, {}, false, ctx);
-
-  // NOTE: state machine might complete before we reach here
-  dst_image_ctx->image_lock.unlock_shared();
-  dst_image_ctx->owner_lock.unlock_shared();
-  if (!sent) {
-    ceph_assert(dst_snap_id == CEPH_NOSNAP);
-    ctx->complete(0);
-  }
-}
-
-template <typename I>
-void ObjectCopyRequest<I>::handle_update_object_map(int r) {
-  ldout(m_cct, 20) << "r=" << r << dendl;
-
-  if (r < 0) {
-    lderr(m_cct) << "failed to update object map: " << cpp_strerror(r) << dendl;
-    finish(r);
-    return;
-  }
-
-  if (!m_dst_object_state.empty()) {
-    send_update_object_map();
-    return;
-  }
   finish(0);
 }
 

diff --git a/src/librbd/deep_copy/ObjectCopyRequest.h b/src/librbd/deep_copy/ObjectCopyRequest.h
index e45685e796e5d54c16d1eb540e2cb627631e9662..e12648132602783fa3e7c0831ffd8b1427693a62 100644 (file)
--- a/src/librbd/deep_copy/ObjectCopyRequest.h
+++ b/src/librbd/deep_copy/ObjectCopyRequest.h
@@ -74,13 +74,13 @@ private:
    *    |     /-----------\
    *    |     |           | (repeat for each snapshot)
    *    v     v           |
-   * WRITE_OBJECT --------/
-   *    |
+   * UPDATE_OBJECT_MAP ---/ (skip if object
+   *    |                    map disabled)
    *    |     /-----------\
    *    |     |           | (repeat for each snapshot)
    *    v     v           |
-   * UPDATE_OBJECT_MAP ---/ (skip if object
-   *    |                    map disabled)
+   * WRITE_OBJECT --------/
+   *    |
    *    v
    * <finish>
    *
@@ -159,12 +159,12 @@ private:
   void send_read();
   void handle_read(int r);
 
-  void send_write_object();
-  void handle_write_object(int r);
-
   void send_update_object_map();
   void handle_update_object_map(int r);
 
+  void send_write_object();
+  void handle_write_object(int r);
+
   Context *start_lock_op(ceph::shared_mutex &owner_lock, int* r);
 
   void compute_read_ops();

diff --git a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc
index 72c194f87bb4c4f70c56bf3d0d002a9301f5286f..e6d569718d83991f59c69a30f35eff694deedd9b 100644 (file)
--- a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc
+++ b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc
@@ -516,10 +516,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Write) {
   expect_list_snaps(mock_src_image_ctx, 0);
   expect_read(mock_src_image_ctx, m_src_snap_ids[0], 0, one.range_end(), 0);
   expect_start_op(mock_exclusive_lock);
-  expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0);
-  expect_start_op(mock_exclusive_lock);
   expect_update_object_map(mock_dst_image_ctx, mock_object_map,
                            m_dst_snap_ids[0], OBJECT_EXISTS, 0);
+  expect_start_op(mock_exclusive_lock);
+  expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0);
 
   request->send();
   ASSERT_EQ(0, ctx.wait());
@@ -587,6 +587,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteError) {
   InSequence seq;
   expect_list_snaps(mock_src_image_ctx, 0);
   expect_read(mock_src_image_ctx, m_src_snap_ids[0], 0, one.range_end(), 0);
+
+  expect_start_op(mock_exclusive_lock);
+  expect_update_object_map(mock_dst_image_ctx, mock_object_map,
+                           m_dst_snap_ids[0], OBJECT_EXISTS, 0);
+
   expect_start_op(mock_exclusive_lock);
   expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, -EINVAL);
 
@@ -636,11 +641,6 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) {
   expect_read(mock_src_image_ctx, m_src_snap_ids[0], 0, one.range_end(), 0);
   expect_read(mock_src_image_ctx, m_src_snap_ids[2], two, 0);
   expect_start_op(mock_exclusive_lock);
-  expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0);
-  expect_start_op(mock_exclusive_lock);
-  expect_write(mock_dst_io_ctx, two,
-               {m_dst_snap_ids[0], {m_dst_snap_ids[0]}}, 0);
-  expect_start_op(mock_exclusive_lock);
   expect_update_object_map(mock_dst_image_ctx, mock_object_map,
                            m_dst_snap_ids[0], OBJECT_EXISTS, 0);
   expect_start_op(mock_exclusive_lock);
@@ -650,6 +650,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) {
   expect_update_object_map(mock_dst_image_ctx, mock_object_map,
                            m_dst_snap_ids[2], is_fast_diff(mock_dst_image_ctx) ?
                            OBJECT_EXISTS_CLEAN : OBJECT_EXISTS, 0);
+  expect_start_op(mock_exclusive_lock);
+  expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0);
+  expect_start_op(mock_exclusive_lock);
+  expect_write(mock_dst_io_ctx, two,
+               {m_dst_snap_ids[0], {m_dst_snap_ids[0]}}, 0);
 
   request->send();
   ASSERT_EQ(0, ctx.wait());
@@ -697,15 +702,15 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Trim) {
   expect_list_snaps(mock_src_image_ctx, 0);
   expect_read(mock_src_image_ctx, m_src_snap_ids[0], 0, one.range_end(), 0);
   expect_start_op(mock_exclusive_lock);
-  expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0);
-  expect_start_op(mock_exclusive_lock);
-  expect_truncate(mock_dst_io_ctx, trim_offset, 0);
-  expect_start_op(mock_exclusive_lock);
   expect_update_object_map(mock_dst_image_ctx, mock_object_map,
                            m_dst_snap_ids[0], OBJECT_EXISTS, 0);
   expect_start_op(mock_exclusive_lock);
   expect_update_object_map(mock_dst_image_ctx, mock_object_map,
                            m_dst_snap_ids[1], OBJECT_EXISTS, 0);
+  expect_start_op(mock_exclusive_lock);
+  expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0);
+  expect_start_op(mock_exclusive_lock);
+  expect_truncate(mock_dst_io_ctx, trim_offset, 0);
 
   request->send();
   ASSERT_EQ(0, ctx.wait());
@@ -748,10 +753,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Remove) {
   InSequence seq;
   expect_list_snaps(mock_src_image_ctx, 0);
   expect_read(mock_src_image_ctx, m_src_snap_ids[1], 0, one.range_end(), 0);
-  expect_start_op(mock_exclusive_lock);
-  expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0);
-  expect_start_op(mock_exclusive_lock);
-  expect_remove(mock_dst_io_ctx, 0);
+
   expect_start_op(mock_exclusive_lock);
   uint8_t state = OBJECT_EXISTS;
   expect_update_object_map(mock_dst_image_ctx, mock_object_map,
@@ -761,6 +763,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Remove) {
                            m_dst_snap_ids[1], is_fast_diff(mock_dst_image_ctx) ?
                            OBJECT_EXISTS_CLEAN : OBJECT_EXISTS, 0);
 
+  expect_start_op(mock_exclusive_lock);
+  expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0);
+  expect_start_op(mock_exclusive_lock);
+  expect_remove(mock_dst_io_ctx, 0);
+
   request->send();
   ASSERT_EQ(0, ctx.wait());
   ASSERT_EQ(0, compare_objects());
@@ -791,15 +798,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ObjectMapUpdateError) {
                                                   mock_dst_image_ctx, 0, 0,
                                                   &ctx);
 
-  librados::MockTestMemIoCtxImpl &mock_dst_io_ctx(get_mock_io_ctx(
-    request->get_dst_io_ctx()));
-
   InSequence seq;
   expect_list_snaps(mock_src_image_ctx, 0);
   expect_read(mock_src_image_ctx, m_src_snap_ids[0], 0, one.range_end(), 0);
   expect_start_op(mock_exclusive_lock);
-  expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0);
-  expect_start_op(mock_exclusive_lock);
   expect_update_object_map(mock_dst_image_ctx, mock_object_map,
                            m_dst_snap_ids[0], OBJECT_EXISTS, -EBLOCKLISTED);
 
@@ -873,14 +875,6 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) {
   expect_read(mock_src_image_ctx, m_src_snap_ids[1], two, 0);
   expect_read(mock_src_image_ctx, m_src_snap_ids[2], three, 0);
 
-  expect_start_op(mock_exclusive_lock);
-  expect_write(mock_dst_io_ctx, two,
-               {m_dst_snap_ids[0], {m_dst_snap_ids[0]}}, 0);
-
-  expect_start_op(mock_exclusive_lock);
-  expect_write(mock_dst_io_ctx, three,
-               {m_dst_snap_ids[1], {m_dst_snap_ids[1], m_dst_snap_ids[0]}}, 0);
-
   expect_start_op(mock_exclusive_lock);
   expect_update_object_map(mock_dst_image_ctx, mock_object_map,
                            m_dst_snap_ids[1], OBJECT_EXISTS, 0);
@@ -889,6 +883,14 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) {
   expect_update_object_map(mock_dst_image_ctx, mock_object_map,
                            CEPH_NOSNAP, OBJECT_EXISTS, 0);
 
+  expect_start_op(mock_exclusive_lock);
+  expect_write(mock_dst_io_ctx, two,
+               {m_dst_snap_ids[0], {m_dst_snap_ids[0]}}, 0);
+
+  expect_start_op(mock_exclusive_lock);
+  expect_write(mock_dst_io_ctx, three,
+               {m_dst_snap_ids[1], {m_dst_snap_ids[1], m_dst_snap_ids[0]}}, 0);
+
   request->send();
   ASSERT_EQ(0, ctx.wait());
   ASSERT_EQ(0, compare_objects());