From 275a299cd48d2ddac36608d6633a6b79c8927351 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Thu, 4 Jan 2024 11:39:20 +0100 Subject: [PATCH] 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 --- src/librbd/object_map/DiffRequest.cc | 23 +++++------- .../object_map/test_mock_DiffRequest.cc | 36 +++++++++++++++++++ 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/src/librbd/object_map/DiffRequest.cc b/src/librbd/object_map/DiffRequest.cc index b8666fb9809ad..0d33078c173d6 100644 --- a/src/librbd/object_map/DiffRequest.cc +++ b/src/librbd/object_map/DiffRequest.cc @@ -212,15 +212,13 @@ void DiffRequest::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::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::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::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()); -- 2.39.5