From f26de2f327b25e1b599add9d4587c68b13050c2d Mon Sep 17 00:00:00 2001 From: Jie Yao Date: Thu, 21 Sep 2023 17:53:54 -0700 Subject: [PATCH] fix comments --- src/lib/homestore/heap_chunk_selector.cpp | 47 +++++++------------ src/lib/homestore/heap_chunk_selector.h | 12 +++-- .../tests/test_heap_chunk_selector.cpp | 20 ++------ 3 files changed, 29 insertions(+), 50 deletions(-) diff --git a/src/lib/homestore/heap_chunk_selector.cpp b/src/lib/homestore/heap_chunk_selector.cpp index e1229647..d48fef52 100644 --- a/src/lib/homestore/heap_chunk_selector.cpp +++ b/src/lib/homestore/heap_chunk_selector.cpp @@ -15,7 +15,17 @@ namespace homeobject { // it means after the single thread initialization, // 1 the key collection of m_per_dev_heap will never change. // 2 the key collection of m_chunks will never change -void HeapChunkSelector::add_chunk(csharedChunk& chunk) { + +// this should only be called when initializing HeapChunkSelector in Homestore +void HeapChunkSelector::add_chunk(csharedChunk& chunk) { m_chunks.emplace(VChunk(chunk).get_chunk_id(), chunk); } + +void HeapChunkSelector::add_chunk_internal(const chunk_num_t chunkID) { + if (m_chunks.find(chunkID) == m_chunks.end()) { + // sanity check + LOGWARN("No chunk found for ChunkID {}", chunkID); + return; + } + const auto& chunk = m_chunks[chunkID]; VChunk vchunk(chunk); auto pdevID = vchunk.get_pdev_id(); // add this find here, since we don`t want to call make_shared in try_emplace every time. @@ -24,8 +34,6 @@ void HeapChunkSelector::add_chunk(csharedChunk& chunk) { auto& avalableBlkCounter = it->second->available_blk_count; avalableBlkCounter.fetch_add(vchunk.available_blks()); - m_chunks.emplace(vchunk.get_chunk_id(), chunk); - auto& heapLock = it->second->mtx; auto& heap = it->second->m_heap; std::lock_guard< std::mutex > l(heapLock); @@ -94,38 +102,19 @@ void HeapChunkSelector::foreach_chunks(std::function< void(csharedChunk&) >&& cb [cb = std::move(cb)](auto& p) { cb(p.second); }); } -void HeapChunkSelector::release_chunk(const homestore::chunk_num_t chunkID) { +void HeapChunkSelector::release_chunk(const chunk_num_t chunkID) { const auto& it = m_chunks.find(chunkID); if (it == m_chunks.end()) { + // sanity check LOGWARN("No chunk found for ChunkID {}", chunkID); } else { - add_chunk(it->second); + add_chunk_internal(chunkID); } } -void HeapChunkSelector::mark_chunk_selected(const homestore::chunk_num_t chunkID) { - const auto& it = m_chunks.find(chunkID); - if (it == m_chunks.end()) { - LOGWARN("No chunk found for ChunkID {}", chunkID); - return; - } - uint32_t pdevID = VChunk(m_chunks[chunkID]).get_pdev_id(); - auto& heapLock = m_per_dev_heap[pdevID]->mtx; - auto& heap = m_per_dev_heap[pdevID]->m_heap; - std::vector< VChunk > temp; - std::lock_guard< std::mutex > l(heapLock); - for (; !heap.empty();) { - VChunk vchunk = heap.top(); - heap.pop(); - if (vchunk.get_chunk_id() == chunkID) { - auto pdevID = vchunk.get_pdev_id(); - m_per_dev_heap[pdevID]->available_blk_count.fetch_sub(vchunk.available_blks()); - break; - } - temp.emplace_back(vchunk); - } - for (auto& vchunk : temp) { - heap.emplace(vchunk); - } +void HeapChunkSelector::build_per_dev_chunk_heap(const std::unordered_set< chunk_num_t >& excludingChunks) { + for (const auto& p : m_chunks) { + if (excludingChunks.find(p.first) == excludingChunks.end()) { add_chunk_internal(p.first); } + }; } } // namespace homeobject \ No newline at end of file diff --git a/src/lib/homestore/heap_chunk_selector.h b/src/lib/homestore/heap_chunk_selector.h index 8e61ba28..20de80c8 100644 --- a/src/lib/homestore/heap_chunk_selector.h +++ b/src/lib/homestore/heap_chunk_selector.h @@ -27,6 +27,7 @@ class HeapChunkSelector : public homestore::ChunkSelector { }; using VChunkHeap = std::priority_queue< VChunk, std::vector< VChunk >, VChunkComparator >; + using chunk_num_t = homestore::chunk_num_t; struct PerDevHeap { std::mutex mtx; @@ -40,16 +41,17 @@ class HeapChunkSelector : public homestore::ChunkSelector { // this function is used to return a chunk back to ChunkSelector when sealing a shard, and will only be used by // Homeobject. - void release_chunk(const homestore::chunk_num_t); + void release_chunk(const chunk_num_t); - // homestore will initialize HeapChunkSelector by adding all the chunks. but some of them are already - // selected by open shards. so after homeobject restarts, we need to mark all these chunks as selected - void mark_chunk_selected(const homestore::chunk_num_t); + // this should be called after ShardManager is initialized and get all the open shards + void build_per_dev_chunk_heap(const std::unordered_set< chunk_num_t >& excludingChunks); private: std::unordered_map< uint32_t, std::shared_ptr< PerDevHeap > > m_per_dev_heap; // hold all the chunks , selected or not - std::unordered_map< homestore::chunk_num_t, csharedChunk > m_chunks; + std::unordered_map< chunk_num_t, csharedChunk > m_chunks; + + void add_chunk_internal(const chunk_num_t); }; } // namespace homeobject \ No newline at end of file diff --git a/src/lib/homestore/tests/test_heap_chunk_selector.cpp b/src/lib/homestore/tests/test_heap_chunk_selector.cpp index 86b8833f..dc927290 100644 --- a/src/lib/homestore/tests/test_heap_chunk_selector.cpp +++ b/src/lib/homestore/tests/test_heap_chunk_selector.cpp @@ -12,6 +12,7 @@ SISL_LOGGING_INIT(logging, HOMEOBJECT_LOG_MODS) SISL_OPTIONS_ENABLE(logging) namespace homestore { + class Chunk : public std::enable_shared_from_this< Chunk > { public: uint32_t available_blks() const { return m_available_blks; } @@ -58,6 +59,7 @@ cshared< Chunk > VChunk::get_internal_chunk() const { return m_internal_chunk->g using homeobject::csharedChunk; using homeobject::HeapChunkSelector; using homestore::Chunk; +using homestore::chunk_num_t; class HeapChunkSelectorTest : public ::testing::Test { protected: @@ -71,6 +73,8 @@ class HeapChunkSelectorTest : public ::testing::Test { HCS.add_chunk(std::make_shared< Chunk >(3, 7, 1)); HCS.add_chunk(std::make_shared< Chunk >(3, 8, 2)); HCS.add_chunk(std::make_shared< Chunk >(3, 9, 3)); + std::unordered_set< chunk_num_t > excludingChunks; + HCS.build_per_dev_chunk_heap(excludingChunks); }; public: @@ -120,22 +124,6 @@ TEST_F(HeapChunkSelectorTest, test_release_chunk) { ASSERT_EQ(chunk2->available_blks(), 2); } -TEST_F(HeapChunkSelectorTest, test_mark_chunk_selected) { - HCS.mark_chunk_selected(2); - homestore::blk_count_t count = 1; - homestore::blk_alloc_hints hints; - hints.pdev_id_hint = 1; - // we mark chunk id 2 is select ,so we will get 3 and 1 - // from pdev 1; - auto chunk = HCS.select_chunk(count, hints); - ASSERT_EQ(chunk->get_pdev_id(), 1); - ASSERT_EQ(chunk->available_blks(), 3); - - chunk = HCS.select_chunk(count, hints); - ASSERT_EQ(chunk->get_pdev_id(), 1); - ASSERT_EQ(chunk->available_blks(), 1); -} - int main(int argc, char* argv[]) { int parsed_argc = argc; ::testing::InitGoogleTest(&parsed_argc, argv);