From: Ilya Dryomov <idryomov@gmail.com>
Date: Thu, 4 Jan 2024 10:39:20 +0000 (+0100)
Subject: librbd/object_map: don't resize object map in handle_load_object_map()
X-Git-Tag: v16.2.15~27^2~6
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=02b29dd21064279fced1334747092458e4231503;p=ceph.git

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

diff --git a/src/librbd/object_map/DiffRequest.cc b/src/librbd/object_map/DiffRequest.cc
index e995b0e31c3f0..3745258fdf66e 100644
--- 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;
   size_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 c7afbe939c296..8d6ee2d27b1cd 100644
--- 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());