]> git.apps.os.sepia.ceph.com Git - ceph-ci.git/commitdiff
librbd: deep-copy should not write to objects that cannot exist
authorJason Dillaman <dillaman@redhat.com>
Thu, 19 Jul 2018 16:34:59 +0000 (12:34 -0400)
committerJason Dillaman <dillaman@redhat.com>
Thu, 19 Jul 2018 17:23:00 +0000 (13:23 -0400)
If an image has a snapshot and is subsequently shrunk, ensure that
discard ops are not sent to the deep-copy objects beyond the bounds
of the image at a given snapshot/HEAD. Only a remove op should be
expected in such cases.

Fixes: http://tracker.ceph.com/issues/25000
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
src/librbd/deep_copy/ObjectCopyRequest.cc
src/librbd/deep_copy/ObjectCopyRequest.h
src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc

index 440cdbed035dceb4eab3030ead4513ab494facdc..19072aee7eb1c34aa5a2425cfbf8adfc1e4a42de 100644 (file)
@@ -61,6 +61,7 @@ ObjectCopyRequest<I>::ObjectCopyRequest(I *src_image_ctx,
   ldout(m_cct, 20) << "dst_oid=" << m_dst_oid << dendl;
 
   compute_src_object_extents();
+  compute_dst_object_may_exist();
 }
 
 template <typename I>
@@ -294,6 +295,7 @@ void ObjectCopyRequest<I>::handle_read_from_parent(int r) {
 template <typename I>
 void ObjectCopyRequest<I>::send_write_object() {
   assert(!m_write_ops.empty());
+  auto& copy_ops = m_write_ops.begin()->second;
 
   // retrieve the destination snap context for the op
   SnapIds dst_snap_ids;
@@ -303,6 +305,15 @@ void ObjectCopyRequest<I>::send_write_object() {
     auto snap_map_it = m_snap_map.find(src_snap_seq);
     assert(snap_map_it != m_snap_map.end());
 
+    auto dst_snap_id = snap_map_it->second.front();
+    auto dst_may_exist_it = m_dst_object_may_exist.find(dst_snap_id);
+    assert(dst_may_exist_it != m_dst_object_may_exist.end());
+    if (!dst_may_exist_it->second && !copy_ops.empty()) {
+      // if the object cannot exist, the only valid op is to remove it
+      assert(copy_ops.size() == 1U);
+      assert(copy_ops.begin()->type == COPY_OP_TYPE_REMOVE);
+    }
+
     // write snapshot context should be before actual snapshot
     if (snap_map_it != m_snap_map.begin()) {
       --snap_map_it;
@@ -318,7 +329,7 @@ void ObjectCopyRequest<I>::send_write_object() {
   librados::ObjectWriteOperation op;
   uint64_t buffer_offset;
 
-  for (auto &copy_op : m_write_ops.begin()->second) {
+  for (auto &copy_op : copy_ops) {
     switch (copy_op.type) {
     case COPY_OP_TYPE_WRITE:
       buffer_offset = 0;
@@ -822,12 +833,22 @@ void ObjectCopyRequest<I>::compute_zero_ops() {
     auto src_snap_seq = it.first;
     auto &zero_interval = it.second;
 
+    auto snap_map_it = m_snap_map.find(src_snap_seq);
+    assert(snap_map_it != m_snap_map.end());
+    auto dst_snap_seq = snap_map_it->second.front();
+
+    auto dst_may_exist_it = m_dst_object_may_exist.find(dst_snap_seq);
+    assert(dst_may_exist_it != m_dst_object_may_exist.end());
+    if (!dst_may_exist_it->second && prev_end_size > 0) {
+      ldout(m_cct, 5) << "object DNE for snap_id: " << dst_snap_seq << dendl;
+      m_write_ops[src_snap_seq].emplace_back(COPY_OP_TYPE_REMOVE, 0, 0, 0);
+      prev_end_size = 0;
+      continue;
+    }
+
     if (hide_parent) {
       RWLock::RLocker snap_locker(m_dst_image_ctx->snap_lock);
       RWLock::RLocker parent_locker(m_dst_image_ctx->parent_lock);
-      auto snap_map_it = m_snap_map.find(src_snap_seq);
-      assert(snap_map_it != m_snap_map.end());
-      auto dst_snap_seq = snap_map_it->second.front();
       uint64_t parent_overlap = 0;
       int r = m_dst_image_ctx->get_parent_overlap(dst_snap_seq, &parent_overlap);
       if (r < 0) {
@@ -920,6 +941,19 @@ void ObjectCopyRequest<I>::finish(int r) {
   on_finish->complete(r);
 }
 
+template <typename I>
+void ObjectCopyRequest<I>::compute_dst_object_may_exist() {
+  RWLock::RLocker snap_locker(m_dst_image_ctx->snap_lock);
+
+  auto snap_ids = m_dst_image_ctx->snaps;
+  snap_ids.push_back(CEPH_NOSNAP);
+
+  for (auto snap_id : snap_ids) {
+    m_dst_object_may_exist[snap_id] =
+      (m_dst_object_number < m_dst_image_ctx->get_object_count(snap_id));
+  }
+}
+
 } // namespace deep_copy
 } // namespace librbd
 
index 0d6598b08c50dfd90f2a7b83e0d11f4c1f38c461..e514d539876af4a0048af54bdad88956387e6143 100644 (file)
@@ -161,6 +161,7 @@ private:
   std::map<librados::snap_t, interval_set<uint64_t>> m_zero_interval;
   std::map<librados::snap_t, interval_set<uint64_t>> m_dst_zero_interval;
   std::map<librados::snap_t, uint8_t> m_dst_object_state;
+  std::map<librados::snap_t, bool> m_dst_object_may_exist;
   bufferlist m_read_from_parent_data;
 
   void send_list_snaps();
@@ -188,6 +189,8 @@ private:
   void merge_write_ops();
   void compute_zero_ops();
 
+  void compute_dst_object_may_exist();
+
   void finish(int r);
 };
 
index cfefb694ec949f70c915378bcf48c92217e773f6..0a1afcd1819d5176ce737d9bc9beb977250b0bc6 100644 (file)
@@ -147,6 +147,13 @@ public:
     mock_image_ctx.exclusive_lock = &mock_exclusive_lock;
   }
 
+  void expect_get_object_count(librbd::MockImageCtx& mock_image_ctx) {
+    EXPECT_CALL(mock_image_ctx, get_object_count(_))
+      .WillRepeatedly(Invoke([&mock_image_ctx](librados::snap_t snap_id) {
+          return mock_image_ctx.image_ctx->get_object_count(snap_id);
+        }));
+  }
+
   void expect_test_features(librbd::MockImageCtx &mock_image_ctx) {
     EXPECT_CALL(mock_image_ctx, test_features(_))
       .WillRepeatedly(WithArg<0>(Invoke([&mock_image_ctx](uint64_t features) {
@@ -198,6 +205,7 @@ public:
       librbd::MockTestImageCtx &mock_src_image_ctx,
       librbd::MockTestImageCtx &mock_dst_image_ctx, Context *on_finish) {
     expect_get_object_name(mock_dst_image_ctx);
+    expect_get_object_count(mock_dst_image_ctx);
     return new MockObjectCopyRequest(&mock_src_image_ctx, nullptr,
                                      &mock_dst_image_ctx, m_snap_map, 0, false,
                                      on_finish);