From 4a0c3dbe405e686fba9661b97a26036a845c3061 Mon Sep 17 00:00:00 2001 From: Harihara Kadayam Date: Wed, 8 Nov 2023 17:41:29 -0800 Subject: [PATCH] Fixed review comments --- conanfile.py | 2 +- src/lib/blkalloc/bitmap_blk_allocator.cpp | 18 ++++++++---------- src/lib/blkalloc/bitmap_blk_allocator.h | 5 +++++ src/lib/blkalloc/blk_allocator.h | 5 ----- src/lib/blkalloc/varsize_blk_allocator.cpp | 1 + src/lib/device/virtual_dev.cpp | 3 ++- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/conanfile.py b/conanfile.py index 5eb61e71c..f39621bbb 100644 --- a/conanfile.py +++ b/conanfile.py @@ -5,7 +5,7 @@ class HomestoreConan(ConanFile): name = "homestore" - version = "4.5.10" + version = "4.5.11" homepage = "https://github.com/eBay/Homestore" description = "HomeStore Storage Engine" diff --git a/src/lib/blkalloc/bitmap_blk_allocator.cpp b/src/lib/blkalloc/bitmap_blk_allocator.cpp index faa7c2e11..8e30c5137 100644 --- a/src/lib/blkalloc/bitmap_blk_allocator.cpp +++ b/src/lib/blkalloc/bitmap_blk_allocator.cpp @@ -31,13 +31,10 @@ BitmapBlkAllocator::BitmapBlkAllocator(BlkAllocConfig const& cfg, bool is_fresh, nullptr); } - if (is_fresh || !is_persistent()) { - m_disk_bm = std::make_unique< sisl::Bitset >(m_num_blks, m_chunk_id, m_align_size); - do_init(); + if (is_fresh) { + if (is_persistent()) { m_disk_bm = std::make_unique< sisl::Bitset >(m_num_blks, m_chunk_id, m_align_size); } } -} -void BitmapBlkAllocator::do_init() { // NOTE: Blocks per portion must be modulo word size so locks do not fall on same word m_blks_per_portion = sisl::round_up(m_blks_per_portion, m_disk_bm ? m_disk_bm->word_size() : 64u); @@ -54,12 +51,12 @@ void BitmapBlkAllocator::on_meta_blk_found(void* mblk_cookie, sisl::byte_view co hs_utils::extract_byte_array(buf, meta_service().is_aligned_buf_needed(size), meta_service().align_size())}}; m_alloced_blk_count.store(m_disk_bm->get_set_count(), std::memory_order_relaxed); - do_init(); - load(); } void BitmapBlkAllocator::cp_flush(CP*) { + if (!is_persistent()) { return; } + if (m_is_disk_bm_dirty.load()) { sisl::byte_array bitmap_buf = acquire_underlying_buffer(); if (m_meta_blk_cookie) { @@ -99,6 +96,8 @@ BlkAllocStatus BitmapBlkAllocator::alloc_on_disk(BlkId const& bid) { { auto lock{portion.portion_auto_lock()}; if (!hs()->is_initializing()) { + // During recovery we might try to free the entry which is already freed while replaying the + // journal, This assert is valid only post recovery. BLKALLOC_REL_ASSERT(m_disk_bm->is_bits_reset(b.blk_num(), b.blk_count()), "Expected disk blks to reset"); } @@ -133,9 +132,8 @@ void BitmapBlkAllocator::free_on_disk(BlkId const& bid) { { auto lock{portion.portion_auto_lock()}; if (!hs()->is_initializing()) { - /* During recovery we might try to free the entry which is already freed while replaying the journal, - * This assert is valid only post recovery. - */ + // During recovery we might try to free the entry which is already freed while replaying the journal, + // This assert is valid only post recovery. if (!m_disk_bm->is_bits_set(b.blk_num(), b.blk_count())) { BLKALLOC_LOG(ERROR, "bit not set {} nblks {} chunk number {}", b.blk_num(), b.blk_count(), m_chunk_id); diff --git a/src/lib/blkalloc/bitmap_blk_allocator.h b/src/lib/blkalloc/bitmap_blk_allocator.h index 92231f5a1..0f048e339 100644 --- a/src/lib/blkalloc/bitmap_blk_allocator.h +++ b/src/lib/blkalloc/bitmap_blk_allocator.h @@ -96,6 +96,10 @@ class BitmapBlkAllocator : public BlkAllocator { /* Get status */ nlohmann::json get_status(int log_level) const override; + void incr_alloced_blk_count(blk_count_t nblks) { m_alloced_blk_count.fetch_add(nblks, std::memory_order_relaxed); } + void decr_alloced_blk_count(blk_count_t nblks) { m_alloced_blk_count.fetch_sub(nblks, std::memory_order_relaxed); } + int64_t get_alloced_blk_count() const { return m_alloced_blk_count.load(std::memory_order_acquire); } + private: void do_init(); sisl::ThreadVector< BlkId >* get_alloc_blk_list(); @@ -117,5 +121,6 @@ class BitmapBlkAllocator : public BlkAllocator { std::unique_ptr< sisl::Bitset > m_disk_bm{nullptr}; std::atomic< bool > m_is_disk_bm_dirty{true}; // initially disk_bm treated as dirty void* m_meta_blk_cookie{nullptr}; + std::atomic< int64_t > m_alloced_blk_count{0}; }; } // namespace homestore \ No newline at end of file diff --git a/src/lib/blkalloc/blk_allocator.h b/src/lib/blkalloc/blk_allocator.h index 172124b1b..e1a65f9f0 100644 --- a/src/lib/blkalloc/blk_allocator.h +++ b/src/lib/blkalloc/blk_allocator.h @@ -166,10 +166,6 @@ class BlkAllocator { virtual std::string to_string() const = 0; virtual void cp_flush(CP* cp) = 0; - void incr_alloced_blk_count(blk_count_t nblks) { m_alloced_blk_count.fetch_add(nblks, std::memory_order_relaxed); } - void decr_alloced_blk_count(blk_count_t nblks) { m_alloced_blk_count.fetch_sub(nblks, std::memory_order_relaxed); } - int64_t get_alloced_blk_count() const { return m_alloced_blk_count.load(std::memory_order_acquire); } - uint32_t get_align_size() const { return m_align_size; } blk_num_t get_total_blks() const { return m_num_blks; } const std::string& get_name() const { return m_name; } @@ -186,7 +182,6 @@ class BlkAllocator { const blk_num_t m_num_blks; const chunk_num_t m_chunk_id; const bool m_is_persistent; - std::atomic< int64_t > m_alloced_blk_count{0}; }; } // namespace homestore diff --git a/src/lib/blkalloc/varsize_blk_allocator.cpp b/src/lib/blkalloc/varsize_blk_allocator.cpp index edc4dbbb9..7174ca246 100644 --- a/src/lib/blkalloc/varsize_blk_allocator.cpp +++ b/src/lib/blkalloc/varsize_blk_allocator.cpp @@ -232,6 +232,7 @@ bool VarsizeBlkAllocator::allocator_state_machine() { } void VarsizeBlkAllocator::load() { + BLKALLOC_DBG_ASSERT_CMP(is_persistent(), ==, true, "Load called on non-persistent blk allocator"); m_cache_bm->copy(*get_disk_bitmap()); BLKALLOC_LOG(INFO, "VarSizeBlkAllocator initialized loading bitmap of size={} used blks={} from persistent storage", diff --git a/src/lib/device/virtual_dev.cpp b/src/lib/device/virtual_dev.cpp index 2f1311002..b9fc7fefc 100644 --- a/src/lib/device/virtual_dev.cpp +++ b/src/lib/device/virtual_dev.cpp @@ -254,7 +254,8 @@ BlkAllocStatus VirtualDev::alloc_blks_from_chunk(blk_count_t nblks, blk_alloc_hi void VirtualDev::free_blk(BlkId const& bid, VDevCPContext* vctx) { auto do_free_action = [this](auto const& b, VDevCPContext* vctx) { - if (vctx) { + if (vctx && (m_allocator_type != blk_allocator_type_t::append)) { + // We don't want to accumulate here for append blk allocator. vctx->m_free_blkid_list.push_back(b); } else { BlkAllocator* allocator = m_dmgr.get_chunk_mutable(b.chunk_num())->blk_allocator_mutable();