rbd-mirror: retry object copy after -ENOENT error

Fixes: http://tracker.ceph.com/issues/18990
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit b4f36d5dc3f4f3cbb23f61cbb945b222248a50df)

diff --git a/src/test/librados_test_stub/MockTestMemIoCtxImpl.h b/src/test/librados_test_stub/MockTestMemIoCtxImpl.h
index 7d3bca3dac671e87f69830d3ff003fe5c5d0b5a8..291853990ebe9eef0ec68e61e510788cc5f87e51 100644 (file)
--- a/src/test/librados_test_stub/MockTestMemIoCtxImpl.h
+++ b/src/test/librados_test_stub/MockTestMemIoCtxImpl.h
@@ -84,6 +84,10 @@ public:
     return TestMemIoCtxImpl::notify(o, bl, timeout_ms, pbl);
   }
 
+  MOCK_METHOD1(set_snap_read, void(snap_t));
+  void do_set_snap_read(snap_t snap_id) {
+    return TestMemIoCtxImpl::set_snap_read(snap_id);
+  }
   MOCK_METHOD5(sparse_read, int(const std::string& oid,
                                uint64_t off,
                                size_t len,
@@ -158,6 +162,7 @@ public:
     ON_CALL(*this, list_watchers(_, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_list_watchers));
     ON_CALL(*this, notify(_, _, _, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_notify));
     ON_CALL(*this, read(_, _, _, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_read));
+    ON_CALL(*this, set_snap_read(_)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_set_snap_read));
     ON_CALL(*this, sparse_read(_, _, _, _, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_sparse_read));
     ON_CALL(*this, remove(_, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_remove));
     ON_CALL(*this, selfmanaged_snap_create(_)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_selfmanaged_snap_create));

diff --git a/src/test/rbd_mirror/image_sync/test_mock_ObjectCopyRequest.cc b/src/test/rbd_mirror/image_sync/test_mock_ObjectCopyRequest.cc
index 5b3006994fd7230f2505895059b300784cfa8fa0..5ded71b8fe840f577d8fc00d5e499f2cfd872329 100644 (file)
--- a/src/test/rbd_mirror/image_sync/test_mock_ObjectCopyRequest.cc
+++ b/src/test/rbd_mirror/image_sync/test_mock_ObjectCopyRequest.cc
@@ -31,11 +31,16 @@ struct MockTestImageCtx : public librbd::MockImageCtx {
 #include "tools/rbd_mirror/image_sync/ObjectCopyRequest.cc"
 template class rbd::mirror::image_sync::ObjectCopyRequest<librbd::MockTestImageCtx>;
 
+bool operator==(const SnapContext& rhs, const SnapContext& lhs) {
+  return (rhs.seq == lhs.seq && rhs.snaps == lhs.snaps);
+}
+
 namespace rbd {
 namespace mirror {
 namespace image_sync {
 
 using ::testing::_;
+using ::testing::DoAll;
 using ::testing::DoDefault;
 using ::testing::InSequence;
 using ::testing::Invoke;
@@ -82,8 +87,21 @@ public:
     ASSERT_EQ(0, open_image(m_local_io_ctx, m_image_name, &m_local_image_ctx));
   }
 
+  void expect_list_snaps(librbd::MockTestImageCtx &mock_image_ctx,
+                         librados::MockTestMemIoCtxImpl &mock_io_ctx,
+                         const librados::snap_set_t &snap_set) {
+    expect_set_snap_read(mock_io_ctx, CEPH_SNAPDIR);
+    EXPECT_CALL(mock_io_ctx,
+                list_snaps(mock_image_ctx.image_ctx->get_object_name(0), _))
+      .WillOnce(DoAll(WithArg<1>(Invoke([&snap_set](librados::snap_set_t *out_snap_set) {
+                          *out_snap_set = snap_set;
+                        })),
+                      Return(0)));
+  }
+
   void expect_list_snaps(librbd::MockTestImageCtx &mock_image_ctx,
                          librados::MockTestMemIoCtxImpl &mock_io_ctx, int r) {
+    expect_set_snap_read(mock_io_ctx, CEPH_SNAPDIR);
     auto &expect = EXPECT_CALL(mock_io_ctx,
                                list_snaps(mock_image_ctx.image_ctx->get_object_name(0),
                                           _));
@@ -109,6 +127,11 @@ public:
                                      0, on_finish);
   }
 
+  void expect_set_snap_read(librados::MockTestMemIoCtxImpl &mock_io_ctx,
+                            uint64_t snap_id) {
+    EXPECT_CALL(mock_io_ctx, set_snap_read(snap_id));
+  }
+
   void expect_sparse_read(librados::MockTestMemIoCtxImpl &mock_io_ctx, uint64_t offset,
                           uint64_t length, int r) {
 
@@ -131,8 +154,9 @@ public:
   }
 
   void expect_write(librados::MockTestMemIoCtxImpl &mock_io_ctx,
-                    uint64_t offset, uint64_t length, int r) {
-    auto &expect = EXPECT_CALL(mock_io_ctx, write(_, _, length, offset, _));
+                    uint64_t offset, uint64_t length,
+                    const SnapContext &snapc, int r) {
+    auto &expect = EXPECT_CALL(mock_io_ctx, write(_, _, length, offset, snapc));
     if (r < 0) {
       expect.WillOnce(Return(r));
     } else {
@@ -141,9 +165,10 @@ public:
   }
 
   void expect_write(librados::MockTestMemIoCtxImpl &mock_io_ctx,
-                    const interval_set<uint64_t> &extents, int r) {
+                    const interval_set<uint64_t> &extents,
+                    const SnapContext &snapc, int r) {
     for (auto extent : extents) {
-      expect_write(mock_io_ctx, extent.first, extent.second, r);
+      expect_write(mock_io_ctx, extent.first, extent.second, snapc, r);
       if (r < 0) {
         break;
       }
@@ -214,6 +239,7 @@ public:
                             m_snap_map.rbegin()->second.end());
     }
     m_snap_map[remote_snap_id] = local_snap_ids;
+    m_remote_snap_ids.push_back(remote_snap_id);
     m_local_snap_ids.push_back(local_snap_id);
 
     return 0;
@@ -300,6 +326,7 @@ public:
   librbd::ImageCtx *m_local_image_ctx;
 
   MockObjectCopyRequest::SnapMap m_snap_map;
+  std::vector<librados::snap_t> m_remote_snap_ids;
   std::vector<librados::snap_t> m_local_snap_ids;
 };
 
@@ -352,8 +379,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Write) {
 
   InSequence seq;
   expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
   expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
-  expect_write(mock_local_io_ctx, 0, one.range_end(), 0);
+  expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0);
   expect_update_object_map(mock_local_image_ctx, mock_object_map,
                            m_local_snap_ids[0], OBJECT_EXISTS, 0);
 
@@ -362,6 +390,99 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Write) {
   ASSERT_EQ(0, compare_objects());
 }
 
+TEST_F(TestMockImageSyncObjectCopyRequest, ReadMissingStaleSnapSet) {
+  ASSERT_EQ(0, create_snap("one"));
+  ASSERT_EQ(0, create_snap("two"));
+
+  // scribble some data
+  interval_set<uint64_t> one;
+  scribble(m_remote_image_ctx, 10, 102400, &one);
+  ASSERT_EQ(0, create_snap("three"));
+
+  ASSERT_EQ(0, create_snap("sync"));
+  librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
+  librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx);
+
+  librbd::MockObjectMap mock_object_map;
+  mock_local_image_ctx.object_map = &mock_object_map;
+
+  expect_test_features(mock_local_image_ctx);
+
+  C_SaferCond ctx;
+  MockObjectCopyRequest *request = create_request(mock_remote_image_ctx,
+                                                  mock_local_image_ctx, &ctx);
+
+  librados::MockTestMemIoCtxImpl &mock_remote_io_ctx(get_mock_io_ctx(
+    request->get_remote_io_ctx()));
+  librados::MockTestMemIoCtxImpl &mock_local_io_ctx(get_mock_io_ctx(
+    request->get_local_io_ctx()));
+
+  librados::clone_info_t dummy_clone_info;
+  dummy_clone_info.cloneid = librados::SNAP_HEAD;
+  dummy_clone_info.size = 123;
+
+  librados::snap_set_t dummy_snap_set1;
+  dummy_snap_set1.clones.push_back(dummy_clone_info);
+
+  dummy_clone_info.size = 234;
+  librados::snap_set_t dummy_snap_set2;
+  dummy_snap_set2.clones.push_back(dummy_clone_info);
+
+  InSequence seq;
+  expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, dummy_snap_set1);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[3]);
+  expect_sparse_read(mock_remote_io_ctx, 0, 123, -ENOENT);
+  expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, dummy_snap_set2);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[3]);
+  expect_sparse_read(mock_remote_io_ctx, 0, 234, -ENOENT);
+  expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[3]);
+  expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
+  expect_write(mock_local_io_ctx, 0, one.range_end(),
+               {m_local_snap_ids[1], {m_local_snap_ids[1],
+                                      m_local_snap_ids[0]}},
+                0);
+  expect_update_object_map(mock_local_image_ctx, mock_object_map,
+                           m_local_snap_ids[2], OBJECT_EXISTS, 0);
+  expect_update_object_map(mock_local_image_ctx, mock_object_map,
+                           m_local_snap_ids[3], OBJECT_EXISTS_CLEAN, 0);
+
+  request->send();
+  ASSERT_EQ(0, ctx.wait());
+  ASSERT_EQ(0, compare_objects());
+}
+
+TEST_F(TestMockImageSyncObjectCopyRequest, ReadMissingUpToDateSnapMap) {
+  // scribble some data
+  interval_set<uint64_t> one;
+  scribble(m_remote_image_ctx, 10, 102400, &one);
+
+  ASSERT_EQ(0, create_snap("sync"));
+  librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
+  librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx);
+
+  librbd::MockObjectMap mock_object_map;
+  mock_local_image_ctx.object_map = &mock_object_map;
+
+  expect_test_features(mock_local_image_ctx);
+
+  C_SaferCond ctx;
+  MockObjectCopyRequest *request = create_request(mock_remote_image_ctx,
+                                                  mock_local_image_ctx, &ctx);
+
+  librados::MockTestMemIoCtxImpl &mock_remote_io_ctx(get_mock_io_ctx(
+    request->get_remote_io_ctx()));
+
+  InSequence seq;
+  expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
+  expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), -ENOENT);
+  expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
+
+  request->send();
+  ASSERT_EQ(-ENOENT, ctx.wait());
+}
+
 TEST_F(TestMockImageSyncObjectCopyRequest, ReadError) {
   // scribble some data
   interval_set<uint64_t> one;
@@ -385,6 +506,7 @@ TEST_F(TestMockImageSyncObjectCopyRequest, ReadError) {
 
   InSequence seq;
   expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
   expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(),  -EINVAL);
 
   request->send();
@@ -416,8 +538,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, WriteError) {
 
   InSequence seq;
   expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
   expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
-  expect_write(mock_local_io_ctx, 0, one.range_end(), -EINVAL);
+  expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, -EINVAL);
 
   request->send();
   ASSERT_EQ(-EINVAL, ctx.wait());
@@ -459,10 +582,13 @@ TEST_F(TestMockImageSyncObjectCopyRequest, WriteSnaps) {
 
   InSequence seq;
   expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
   expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
-  expect_write(mock_local_io_ctx, 0, one.range_end(), 0);
+  expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[2]);
   expect_sparse_read(mock_remote_io_ctx, two, 0);
-  expect_write(mock_local_io_ctx, two, 0);
+  expect_write(mock_local_io_ctx, two,
+               {m_local_snap_ids[0], {m_local_snap_ids[0]}}, 0);
   expect_update_object_map(mock_local_image_ctx, mock_object_map,
                            m_local_snap_ids[0], OBJECT_EXISTS, 0);
   expect_update_object_map(mock_local_image_ctx, mock_object_map,
@@ -508,8 +634,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Trim) {
 
   InSequence seq;
   expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
   expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
-  expect_write(mock_local_io_ctx, 0, one.range_end(), 0);
+  expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0);
   expect_truncate(mock_local_io_ctx, trim_offset, 0);
   expect_update_object_map(mock_local_image_ctx, mock_object_map,
                            m_local_snap_ids[0], OBJECT_EXISTS, 0);
@@ -526,6 +653,7 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Remove) {
   interval_set<uint64_t> one;
   scribble(m_remote_image_ctx, 10, 102400, &one);
   ASSERT_EQ(0, create_snap("one"));
+  ASSERT_EQ(0, create_snap("two"));
 
   // remove the object
   uint64_t object_size = 1 << m_remote_image_ctx->order;
@@ -550,11 +678,14 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Remove) {
 
   InSequence seq;
   expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
+  expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[1]);
   expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
-  expect_write(mock_local_io_ctx, 0, one.range_end(), 0);
+  expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0);
   expect_remove(mock_local_io_ctx, 0);
   expect_update_object_map(mock_local_image_ctx, mock_object_map,
                            m_local_snap_ids[0], OBJECT_EXISTS, 0);
+  expect_update_object_map(mock_local_image_ctx, mock_object_map,
+                           m_local_snap_ids[1], OBJECT_EXISTS_CLEAN, 0);
 
   request->send();
   ASSERT_EQ(0, ctx.wait());

diff --git a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc
index 5ebfa5c38609b278a7a093de9add3a66c6d554e3..9bf4381d826cee58c7f7fcc2595e454ee6ce18c4 100644 (file)
--- a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc
+++ b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc
@@ -12,6 +12,22 @@
 #define dout_prefix *_dout << "rbd::mirror::image_sync::ObjectCopyRequest: " \
                            << this << " " << __func__
 
+namespace librados {
+
+bool operator==(const clone_info_t& rhs, const clone_info_t& lhs) {
+  return (rhs.cloneid == lhs.cloneid &&
+          rhs.snaps == lhs.snaps &&
+          rhs.overlap == lhs.overlap &&
+          rhs.size == lhs.size);
+}
+
+bool operator==(const snap_set_t& rhs, const snap_set_t& lhs) {
+  return (rhs.clones == lhs.clones &&
+          rhs.seq == lhs.seq);
+}
+
+} // namespace librados
+
 namespace rbd {
 namespace mirror {
 namespace image_sync {
@@ -54,6 +70,8 @@ void ObjectCopyRequest<I>::send_list_snaps() {
     ObjectCopyRequest<I>, &ObjectCopyRequest<I>::handle_list_snaps>(this);
 
   librados::ObjectReadOperation op;
+  m_snap_set = {};
+  m_snap_ret = 0;
   op.list_snaps(&m_snap_set, &m_snap_ret);
 
   m_remote_io_ctx.snap_set_read(CEPH_SNAPDIR);
@@ -75,12 +93,26 @@ void ObjectCopyRequest<I>::handle_list_snaps(int r) {
     finish(0);
     return;
   }
+
   if (r < 0) {
     derr << ": failed to list snaps: " << cpp_strerror(r) << dendl;
     finish(r);
     return;
   }
 
+  if (m_retry_missing_read) {
+    if (m_snap_set == m_retry_snap_set) {
+      derr << ": read encountered missing object using up-to-date snap set"
+           << dendl;
+      finish(-ENOENT);
+      return;
+    }
+
+    dout(20) << ": retrying using updated snap set" << dendl;
+    m_retry_missing_read = false;
+    m_retry_snap_set = {};
+  }
+
   compute_diffs();
   send_read_object();
 }
@@ -97,16 +129,17 @@ void ObjectCopyRequest<I>::send_read_object() {
   auto &sync_ops = m_snap_sync_ops.begin()->second;
   assert(!sync_ops.empty());
 
-  // map the sync op start snap id back to the necessary read snap id
-  librados::snap_t remote_snap_seq = m_snap_sync_ops.begin()->first.second;
-  m_remote_io_ctx.snap_set_read(remote_snap_seq);
-
   bool read_required = false;
   librados::ObjectReadOperation op;
   for (auto &sync_op : sync_ops) {
     switch (sync_op.type) {
     case SYNC_OP_TYPE_WRITE:
       if (!read_required) {
+        // map the sync op start snap id back to the necessary read snap id
+        librados::snap_t remote_snap_seq =
+          m_snap_sync_ops.begin()->first.second;
+        m_remote_io_ctx.snap_set_read(remote_snap_seq);
+
         dout(20) << ": remote_snap_seq=" << remote_snap_seq << dendl;
         read_required = true;
       }
@@ -139,6 +172,15 @@ template <typename I>
 void ObjectCopyRequest<I>::handle_read_object(int r) {
   dout(20) << ": r=" << r << dendl;
 
+  if (r == -ENOENT) {
+    m_retry_snap_set = m_snap_set;
+    m_retry_missing_read = true;
+
+    dout(5) << ": object missing potentially due to removed snapshot" << dendl;
+    send_list_snaps();
+    return;
+  }
+
   if (r < 0) {
     derr << ": failed to read from remote object: " << cpp_strerror(r)
          << dendl;
@@ -313,6 +355,10 @@ template <typename I>
 void ObjectCopyRequest<I>::compute_diffs() {
   CephContext *cct = m_local_image_ctx->cct;
 
+  m_snap_sync_ops = {};
+  m_snap_object_states = {};
+  m_snap_object_sizes = {};
+
   librados::snap_t remote_sync_pont_snap_id = m_snap_map->rbegin()->first;
   uint64_t prev_end_size = 0;
   bool prev_exists = false;

diff --git a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h
index 17545d301987bd8c5fd9cbfe17398335571be8eb..78068861a5c0caf1d1247a4c95b58e977e5f58cc 100644 (file)
--- a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h
+++ b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h
@@ -54,9 +54,11 @@ private:
    * <start>
    *    |
    *    v
-   * LIST_SNAPS
-   *    |
-   *    v
+   * LIST_SNAPS < * * *
+   *    |             * (-ENOENT and snap set stale)
+   *    |   * * * * * *
+   *    |   *
+   *    v   *
    * READ_OBJECT <--------\
    *    |                 | (repeat for each snapshot)
    *    v                 |
@@ -115,6 +117,9 @@ private:
   librados::snap_set_t m_snap_set;
   int m_snap_ret;
 
+  bool m_retry_missing_read = false;
+  librados::snap_set_t m_retry_snap_set;
+
   SnapSyncOps m_snap_sync_ops;
   SnapObjectStates m_snap_object_states;
   SnapObjectSizes m_snap_object_sizes;