librbd/object_map: don't resize object map in handle_load_object_map()

Currently it's done in two cases:

- if the loaded object map is larger than expected based on byte size,
  it's truncated to expected number of objects
- in case of deep-copy, if the loaded object map is smaller than diff
  state, it's expanded to get "track the largest of all versions in the
  set" semantics

Both of these cases can be easily dealt with without modifying the
object map.  Being able to process a const object map is needed for
working on in-memory object map which is external to DiffRequest.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

diff --git a/src/librbd/object_map/DiffRequest.cc b/src/librbd/object_map/DiffRequest.cc
index b8666fb9809addf8b4715545cdb46d329d72d0ee..0d33078c173d6913c489bac3fa4412ad2c43d13d 100644 (file)
--- a/src/librbd/object_map/DiffRequest.cc
+++ b/src/librbd/object_map/DiffRequest.cc
@@ -212,15 +212,13 @@ void DiffRequest<I>::handle_load_object_map(int r) {
                   << m_object_map.size() << " < " << num_objs << dendl;
     finish(-EINVAL);
     return;
-  } else {
-    m_object_map.resize(num_objs);
   }
 
   uint64_t start_object_no, end_object_no;
   uint64_t prev_object_diff_state_size = m_object_diff_state->size();
   if (is_diff_iterate()) {
-    start_object_no = std::min(m_start_object_no, m_object_map.size());
-    end_object_no = std::min(m_end_object_no, m_object_map.size());
+    start_object_no = std::min(m_start_object_no, num_objs);
+    end_object_no = std::min(m_end_object_no, num_objs);
     uint64_t num_objs_in_range = end_object_no - start_object_no;
     if (m_object_diff_state->size() != num_objs_in_range) {
       m_object_diff_state->resize(num_objs_in_range);
@@ -228,13 +226,10 @@ void DiffRequest<I>::handle_load_object_map(int r) {
   } else {
     // for deep-copy, the object diff state should be the largest of
     // all versions in the set, so it's only ever grown
-    if (m_object_diff_state->size() < m_object_map.size()) {
-      m_object_diff_state->resize(m_object_map.size());
-    } else if (m_object_diff_state->size() > m_object_map.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
-      // diff state
-      m_object_map.resize(m_object_diff_state->size());
+    // shrink is handled by flagging trimmed objects as non-existent
+    // and comparing against the previous object diff state as usual
+    if (m_object_diff_state->size() < num_objs) {
+      m_object_diff_state->resize(num_objs);
     }
     start_object_no = 0;
     end_object_no = m_object_diff_state->size();
@@ -245,8 +240,8 @@ void DiffRequest<I>::handle_load_object_map(int r) {
   auto it = m_object_map.begin() + start_object_no;
   auto diff_it = m_object_diff_state->begin();
   uint64_t ono = start_object_no;
-  for (; ono < start_object_no + overlap; ++it, ++diff_it, ++ono) {
-    uint8_t object_map_state = *it;
+  for (; ono < start_object_no + overlap; ++diff_it, ++ono) {
+    uint8_t object_map_state = (ono < num_objs ? *it++ : OBJECT_NONEXISTENT);
     uint8_t prev_object_diff_state = *diff_it;
     switch (prev_object_diff_state) {
     case DIFF_STATE_HOLE:
@@ -287,7 +282,7 @@ void DiffRequest<I>::handle_load_object_map(int r) {
   ldout(cct, 20) << "computed overlap diffs" << dendl;
 
   ceph_assert(diff_it == m_object_diff_state->end() ||
-              end_object_no <= m_object_map.size());
+              end_object_no <= num_objs);
   for (; ono < end_object_no; ++it, ++diff_it, ++ono) {
     uint8_t object_map_state = *it;
     if (object_map_state == OBJECT_NONEXISTENT) {

diff --git a/src/test/librbd/object_map/test_mock_DiffRequest.cc b/src/test/librbd/object_map/test_mock_DiffRequest.cc
index c7afbe939c296714bf9280900b08e4c87e26c41f..8d6ee2d27b1cdab4e6c7258e6a378d8246de62a2 100644 (file)
--- a/src/test/librbd/object_map/test_mock_DiffRequest.cc
+++ b/src/test/librbd/object_map/test_mock_DiffRequest.cc
@@ -1963,6 +1963,42 @@ TEST_P(TestMockObjectMapDiffRequest, IntermediateObjectMapTooSmallFromSnap) {
   }
 }
 
+TEST_P(TestMockObjectMapDiffRequest, ObjectMapTooLarge) {
+  REQUIRE_FEATURE(RBD_FEATURE_FAST_DIFF);
+
+  uint32_t object_count = 5;
+  m_image_ctx->size = object_count * (1 << m_image_ctx->order);
+  m_image_ctx->snap_info = {
+    {1U, {"snap1", {cls::rbd::UserSnapshotNamespace{}}, m_image_ctx->size, {},
+          {}, {}, {}}}
+  };
+
+  BitVector<2> object_map_1;
+  object_map_1.resize(object_count + 12);
+  BitVector<2> object_map_head;
+  object_map_head.resize(object_count + 34);
+  object_map_head[1] = OBJECT_EXISTS_CLEAN;
+  BitVector<2> expected_diff_state;
+  expected_diff_state.resize(object_count);
+  expected_diff_state[1] = DIFF_STATE_DATA_UPDATED;
+
+  if (is_diff_iterate()) {
+    auto load = [&](MockTestImageCtx& mock_image_ctx) {
+      expect_get_flags(mock_image_ctx, CEPH_NOSNAP, 0, 0);
+      expect_load_map(mock_image_ctx, CEPH_NOSNAP, object_map_head, 0);
+    };
+    test_diff_iterate(load, 0, CEPH_NOSNAP, expected_diff_state);
+  } else {
+    auto load = [&](MockTestImageCtx& mock_image_ctx) {
+      expect_get_flags(mock_image_ctx, 1, 0, 0);
+      expect_load_map(mock_image_ctx, 1, object_map_1, 0);
+      expect_get_flags(mock_image_ctx, CEPH_NOSNAP, 0, 0);
+      expect_load_map(mock_image_ctx, CEPH_NOSNAP, object_map_head, 0);
+    };
+    test_deep_copy(load, 0, CEPH_NOSNAP, expected_diff_state);
+  }
+}
+
 INSTANTIATE_TEST_SUITE_P(MockObjectMapDiffRequestTests,
                          TestMockObjectMapDiffRequest, ::testing::Bool());