From 38c7ec427fd2a90363b987d80579e5904dc8f8c9 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Thu, 28 Jan 2021 18:30:16 -0500 Subject: [PATCH] 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 (cherry picked from commit b81cd2460de748c71210520f8c819895f257f0c7) Conflicts: src/librbd/api/DiffIterate.cc: trivial resolution due to renames --- src/librbd/api/DiffIterate.cc | 15 ++--- src/librbd/deep_copy/ImageCopyRequest.cc | 2 +- src/librbd/object_map/DiffRequest.cc | 61 +++++++++++-------- src/librbd/object_map/DiffRequest.h | 3 +- src/librbd/object_map/Types.h | 7 ++- .../object_map/test_mock_DiffRequest.cc | 19 +++--- 6 files changed, 57 insertions(+), 50 deletions(-) diff --git a/src/librbd/api/DiffIterate.cc b/src/librbd/api/DiffIterate.cc index 7e47c7ec3a82e..527193b90533c 100644 --- 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::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::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::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 17aaa533efa4b..5d28c7f76be1d 100644 --- a/src/librbd/deep_copy/ImageCopyRequest.cc +++ b/src/librbd/deep_copy/ImageCopyRequest.cc @@ -186,7 +186,7 @@ int ImageCopyRequest::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 1058288a6bac3..a8331bc5afc47 100644 --- a/src/librbd/object_map/DiffRequest.cc +++ b/src/librbd/object_map/DiffRequest.cc @@ -175,58 +175,67 @@ void DiffRequest::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(*pre_it) - << "->" << static_cast(*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(prev_object_diff_state) + << "->" << static_cast(*diff_it) << " (" + << static_cast(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(*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(*diff_it) << " (" + << static_cast(*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 31a7a1bea2505..e83a1629e6233 100644 --- 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 b3741929d771b..0ce91bd96a1c1 100644 --- 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 366bbb2786034..f350d35e3866b 100644 --- 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); } -- 2.39.5