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>
(cherry picked from commit e782b85bfda8ae6487c637af0059ab94fba332d6)

Conflicts:
	src/librbd/deep_copy/ObjectCopyRequest.cc: trivial resolution
	src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc: trivial resolution

diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc
index 4e8d23d53fb6af103e1f258e5c7eeef3e7649496..dd5da5f31a16d27e129f7b44b2b4d5afabfa1128 100644 (file)
--- a/src/librbd/deep_copy/ObjectCopyRequest.cc
+++ b/src/librbd/deep_copy/ObjectCopyRequest.cc
@@ -307,10 +307,87 @@ void ObjectCopyRequest<I>::handle_read_from_parent(int r) {
     return;
   }
 
-  send_write_object();
+  send_update_object_map();
   return;
 }
 
+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());
@@ -444,82 +521,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 5d57a8b32c222c7ef8018a875f6b782f0d13328c..c39c33b9b6326a3ec851b64c0b60581bd73203fb 100644 (file)
--- a/src/librbd/deep_copy/ObjectCopyRequest.h
+++ b/src/librbd/deep_copy/ObjectCopyRequest.h
@@ -80,13 +80,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>
    *
@@ -185,12 +185,12 @@ private:
   void send_read_from_parent();
   void handle_read_from_parent(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);
 
   uint64_t src_to_dst_object_offset(uint64_t objectno, uint64_t offset);

diff --git a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc
index c2e03db37b715ac7353ec564efbcdc57017f044d..dc1c4f323c1af8838298d37e92215841ebc00bd8 100644 (file)
--- a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc
+++ b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc
@@ -526,10 +526,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Write) {
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]);
   expect_sparse_read(mock_src_io_ctx, 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());
@@ -581,19 +581,20 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadMissingStaleSnapSet) {
 
   InSequence seq;
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, dummy_snap_set1);
+
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[3]);
   expect_sparse_read(mock_src_io_ctx, 0, 123, -ENOENT);
+
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, dummy_snap_set2);
+
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[3]);
   expect_sparse_read(mock_src_io_ctx, 0, 234, -ENOENT);
+
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0);
+
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[3]);
   expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), 0);
-  expect_start_op(mock_exclusive_lock);
-  expect_write(mock_dst_io_ctx, 0, one.range_end(),
-               {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[2], OBJECT_EXISTS, 0);
@@ -602,6 +603,12 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadMissingStaleSnapSet) {
                            m_dst_snap_ids[3], 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(),
+               {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());
@@ -635,6 +642,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadMissingUpToDateSnapMap) {
   InSequence seq;
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0);
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]);
+
   expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), -ENOENT);
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0);
 
@@ -708,6 +716,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteError) {
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0);
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]);
   expect_sparse_read(mock_src_io_ctx, 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);
 
@@ -756,15 +769,13 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) {
 
   InSequence seq;
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0);
+
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]);
   expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), 0);
+
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[2]);
   expect_sparse_read(mock_src_io_ctx, 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);
@@ -776,6 +787,12 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) {
                            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());
   ASSERT_EQ(0, compare_objects());
@@ -822,12 +839,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Trim) {
 
   InSequence seq;
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0);
+
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]);
   expect_sparse_read(mock_src_io_ctx, 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);
@@ -835,6 +850,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Trim) {
   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());
   ASSERT_EQ(0, compare_objects());
@@ -877,11 +897,9 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Remove) {
   InSequence seq;
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0);
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[1]);
+
   expect_sparse_read(mock_src_io_ctx, 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,
@@ -891,6 +909,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());
@@ -928,10 +951,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ObjectMapUpdateError) {
 
   InSequence seq;
   expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0);
+
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]);
   expect_sparse_read(mock_src_io_ctx, 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, -EBLACKLISTED);
@@ -1013,14 +1036,6 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) {
   expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[2]);
   expect_sparse_read(mock_src_io_ctx, 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);
@@ -1029,6 +1044,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());