librbd/object_map: diff state machine should track object existence

The deep-copy snapshot-create state machine initializes the object-map
state to non-existent for all objects. There was an assumption that the
deep-copy object-copy state machine would always update the object map
but that was being skipped for clean objects as an optimization. This
change will support a future commit to run the object-copy state machine
for existing objects.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit b81cd2460de748c71210520f8c819895f257f0c7)

Conflicts:
	src/librbd/api/DiffIterate.cc: trivial resolution due to renames

diff --git a/src/librbd/api/DiffIterate.cc b/src/librbd/api/DiffIterate.cc
index 7e47c7ec3a82ec6d0879fdae3314a86ec224bfd1..527193b90533c15b9308aad641d1f017168e979c 100644 (file)
--- a/src/librbd/api/DiffIterate.cc
+++ b/src/librbd/api/DiffIterate.cc
@@ -32,12 +32,6 @@ namespace api {
 
 namespace {
 
-enum ObjectDiffState {
-  OBJECT_DIFF_STATE_NONE    = 0,
-  OBJECT_DIFF_STATE_UPDATED = 1,
-  OBJECT_DIFF_STATE_HOLE    = 2
-};
-
 struct DiffContext {
   DiffIterate<>::Callback callback;
   void *callback_arg;
@@ -381,7 +375,8 @@ int DiffIterate<I>::execute() {
 
       if (fast_diff_enabled) {
         const uint64_t object_no = p->second.front().objectno;
-        if (object_diff_state[object_no] == OBJECT_DIFF_STATE_NONE &&
+        uint8_t diff_state = object_diff_state[object_no];
+        if (diff_state == object_map::DIFF_STATE_HOLE &&
             from_snap_id == 0 && !diff_context.parent_diff.empty()) {
           // no data in child object -- report parent diff instead
           for (auto& oe : p->second) {
@@ -399,9 +394,9 @@ int DiffIterate<I>::execute() {
               }
             }
           }
-        } else if (object_diff_state[object_no] != OBJECT_DIFF_STATE_NONE) {
-          bool updated = (object_diff_state[object_no] ==
-                            OBJECT_DIFF_STATE_UPDATED);
+        } else if (diff_state == object_map::DIFF_STATE_HOLE_UPDATED ||
+                   diff_state == object_map::DIFF_STATE_DATA_UPDATED) {
+          bool updated = (diff_state == object_map::DIFF_STATE_DATA_UPDATED);
           for (std::vector<ObjectExtent>::iterator q = p->second.begin();
                q != p->second.end(); ++q) {
             r = m_callback(off + q->offset, q->length, updated, m_callback_arg);

diff --git a/src/librbd/deep_copy/ImageCopyRequest.cc b/src/librbd/deep_copy/ImageCopyRequest.cc
index 17aaa533efa4b4f73730c3c04db411f2a7b45bec..5d28c7f76be1da4b262797c59d30f3a7ad78d437 100644 (file)
--- a/src/librbd/deep_copy/ImageCopyRequest.cc
+++ b/src/librbd/deep_copy/ImageCopyRequest.cc
@@ -186,7 +186,7 @@ int ImageCopyRequest<I>::send_next_object_copy() {
     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_NONE) {
+          m_object_diff_state[src_ono] != object_map::DIFF_STATE_HOLE) {
         skip = false;
         break;
       }

diff --git a/src/librbd/object_map/DiffRequest.cc b/src/librbd/object_map/DiffRequest.cc
index 1058288a6bac331c44533ca32a9760267a793917..a8331bc5afc47490be536917cebdac7f18c55832 100644 (file)
--- a/src/librbd/object_map/DiffRequest.cc
+++ b/src/librbd/object_map/DiffRequest.cc
@@ -175,58 +175,67 @@ void DiffRequest<I>::handle_load_object_map(int r) {
     m_object_map.resize(num_objs);
   }
 
-  if (m_object_diff_state->size() < num_objs) {
+  size_t prev_object_diff_state_size = m_object_diff_state->size();
+  if (prev_object_diff_state_size < num_objs) {
     // the diff state should be the largest of all snapshots in the set
     m_object_diff_state->resize(num_objs);
   }
   if (m_object_map.size() < m_object_diff_state->size()) {
     // the image was shrunk so expanding the object map will flag end objects
     // as non-existent and they will be compared against the previous object
-    // map
+    // diff state
     m_object_map.resize(m_object_diff_state->size());
   }
 
-  uint64_t overlap = std::min(m_object_map.size(), m_prev_object_map.size());
+  uint64_t overlap = std::min(m_object_map.size(), prev_object_diff_state_size);
   auto it = m_object_map.begin();
   auto overlap_end_it = it + overlap;
-  auto pre_it = m_prev_object_map.begin();
   auto diff_it = m_object_diff_state->begin();
   uint64_t i = 0;
-  for (; it != overlap_end_it; ++it, ++pre_it, ++diff_it, ++i) {
-    ldout(cct, 20) << "object state: " << i << " "
-                   << static_cast<uint32_t>(*pre_it)
-                   << "->" << static_cast<uint32_t>(*it) << dendl;
-    if (*it == OBJECT_NONEXISTENT) {
-      if (*pre_it != OBJECT_NONEXISTENT) {
-        *diff_it = DIFF_STATE_HOLE;
-      }
-    } else if (*it == OBJECT_EXISTS ||
-               (*pre_it != *it &&
-                !(*pre_it == OBJECT_EXISTS &&
-                  *it == OBJECT_EXISTS_CLEAN))) {
-      *diff_it = DIFF_STATE_UPDATED;
+  for (; it != overlap_end_it; ++it, ++diff_it, ++i) {
+    uint8_t object_map_state = *it;
+    uint8_t prev_object_diff_state = *diff_it;
+    if (object_map_state == OBJECT_EXISTS ||
+        object_map_state == OBJECT_PENDING ||
+        (object_map_state == OBJECT_EXISTS_CLEAN &&
+         prev_object_diff_state != DIFF_STATE_DATA &&
+         prev_object_diff_state != DIFF_STATE_DATA_UPDATED)) {
+      *diff_it = DIFF_STATE_DATA_UPDATED;
+    } else if (object_map_state == OBJECT_NONEXISTENT &&
+               prev_object_diff_state != DIFF_STATE_HOLE &&
+               prev_object_diff_state != DIFF_STATE_HOLE_UPDATED) {
+      *diff_it = DIFF_STATE_HOLE_UPDATED;
     }
+
+    ldout(cct, 20) << "object state: " << i << " "
+                   << static_cast<uint32_t>(prev_object_diff_state)
+                   << "->" << static_cast<uint32_t>(*diff_it) << " ("
+                   << static_cast<uint32_t>(object_map_state) << ")"
+                   << dendl;
   }
   ldout(cct, 20) << "computed overlap diffs" << dendl;
 
   bool diff_from_start = (m_snap_id_start == 0);
   auto end_it = m_object_map.end();
-  if (m_object_map.size() > m_prev_object_map.size() &&
-      (diff_from_start || m_prev_object_map_valid)) {
+  if (m_object_map.size() > prev_object_diff_state_size) {
     for (; it != end_it; ++it,++diff_it, ++i) {
-      ldout(cct, 20) << "object state: " << i << " "
-                     << "->" << static_cast<uint32_t>(*it) << dendl;
-      if (*it == OBJECT_NONEXISTENT) {
-        *diff_it = DIFF_STATE_NONE;
+      uint8_t object_map_state = *it;
+      if (object_map_state == OBJECT_NONEXISTENT) {
+        *diff_it = DIFF_STATE_HOLE;
+      } else if (diff_from_start || object_map_state != OBJECT_EXISTS_CLEAN) {
+        *diff_it = DIFF_STATE_DATA_UPDATED;
       } else {
-        *diff_it = DIFF_STATE_UPDATED;
+        *diff_it = DIFF_STATE_DATA;
       }
+
+      ldout(cct, 20) << "object state: " << i << " "
+                     << "->" << static_cast<uint32_t>(*diff_it) << " ("
+                     << static_cast<uint32_t>(*it) << ")" << dendl;
     }
   }
   ldout(cct, 20) << "computed resize diffs" << dendl;
 
-  m_prev_object_map = m_object_map;
-  m_prev_object_map_valid = true;
+  m_object_diff_state_valid = true;
 
   std::shared_lock image_locker{m_image_ctx->image_lock};
   load_object_map(&image_locker);

diff --git a/src/librbd/object_map/DiffRequest.h b/src/librbd/object_map/DiffRequest.h
index 31a7a1bea25056fd51c101a86b049a2210892c7d..e83a1629e62339023c81090a9f7e3ec767d54cde 100644 (file)
--- a/src/librbd/object_map/DiffRequest.h
+++ b/src/librbd/object_map/DiffRequest.h
@@ -68,8 +68,7 @@ private:
   uint64_t m_current_size = 0;
 
   BitVector<2> m_object_map;
-  BitVector<2> m_prev_object_map;
-  bool m_prev_object_map_valid = false;
+  bool m_object_diff_state_valid = false;
 
   bufferlist m_out_bl;
 

diff --git a/src/librbd/object_map/Types.h b/src/librbd/object_map/Types.h
index b3741929d771bf485ad985174a3d76b1aaf71448..0ce91bd96a1c140ecc0cf94f5e9c49bcb5984ce2 100644 (file)
--- a/src/librbd/object_map/Types.h
+++ b/src/librbd/object_map/Types.h
@@ -8,9 +8,10 @@ namespace librbd {
 namespace object_map {
 
 enum DiffState {
-  DIFF_STATE_NONE    = 0,
-  DIFF_STATE_UPDATED = 1,
-  DIFF_STATE_HOLE    = 2
+  DIFF_STATE_HOLE         = 0, /* unchanged hole */
+  DIFF_STATE_DATA         = 1, /* unchanged data */
+  DIFF_STATE_HOLE_UPDATED = 2, /* new hole */
+  DIFF_STATE_DATA_UPDATED = 3  /* new data */
 };
 
 } // namespace object_map

diff --git a/src/test/librbd/object_map/test_mock_DiffRequest.cc b/src/test/librbd/object_map/test_mock_DiffRequest.cc
index 366bbb27860343f3240a59cae3df4c57696a9abb..f350d35e3866b1abb3a07ea863917b15296e494f 100644 (file)
--- a/src/test/librbd/object_map/test_mock_DiffRequest.cc
+++ b/src/test/librbd/object_map/test_mock_DiffRequest.cc
@@ -186,9 +186,9 @@ TEST_F(TestMockObjectMapDiffRequest, FullDelta) {
 
   BitVector<2> expected_diff_state;
   expected_diff_state.resize(object_count);
-  expected_diff_state[1] = DIFF_STATE_UPDATED;
-  expected_diff_state[2] = DIFF_STATE_UPDATED;
-  expected_diff_state[3] = DIFF_STATE_HOLE;
+  expected_diff_state[1] = DIFF_STATE_DATA_UPDATED;
+  expected_diff_state[2] = DIFF_STATE_DATA_UPDATED;
+  expected_diff_state[3] = DIFF_STATE_HOLE_UPDATED;
   ASSERT_EQ(expected_diff_state, m_object_diff_state);
 }
 
@@ -233,8 +233,9 @@ TEST_F(TestMockObjectMapDiffRequest, IntermediateDelta) {
 
   BitVector<2> expected_diff_state;
   expected_diff_state.resize(object_count);
-  expected_diff_state[2] = DIFF_STATE_UPDATED;
-  expected_diff_state[3] = DIFF_STATE_UPDATED;
+  expected_diff_state[1] = DIFF_STATE_DATA;
+  expected_diff_state[2] = DIFF_STATE_DATA_UPDATED;
+  expected_diff_state[3] = DIFF_STATE_DATA_UPDATED;
   ASSERT_EQ(expected_diff_state, m_object_diff_state);
 }
 
@@ -279,7 +280,9 @@ TEST_F(TestMockObjectMapDiffRequest, EndDelta) {
 
   BitVector<2> expected_diff_state;
   expected_diff_state.resize(object_count);
-  expected_diff_state[3] = DIFF_STATE_HOLE;
+  expected_diff_state[1] = DIFF_STATE_DATA;
+  expected_diff_state[2] = DIFF_STATE_DATA_UPDATED;
+  expected_diff_state[3] = DIFF_STATE_HOLE_UPDATED;
   ASSERT_EQ(expected_diff_state, m_object_diff_state);
 }
 
@@ -370,7 +373,7 @@ TEST_F(TestMockObjectMapDiffRequest, IntermediateSnapDNE) {
 
   BitVector<2> expected_diff_state;
   expected_diff_state.resize(object_count);
-  expected_diff_state[1] = DIFF_STATE_UPDATED;
+  expected_diff_state[1] = DIFF_STATE_DATA_UPDATED;
   ASSERT_EQ(expected_diff_state, m_object_diff_state);
 }
 
@@ -430,7 +433,7 @@ TEST_F(TestMockObjectMapDiffRequest, LoadIntermediateObjectMapDNE) {
 
   BitVector<2> expected_diff_state;
   expected_diff_state.resize(object_count);
-  expected_diff_state[1] = DIFF_STATE_UPDATED;
+  expected_diff_state[1] = DIFF_STATE_DATA_UPDATED;
   ASSERT_EQ(expected_diff_state, m_object_diff_state);
 }