From 664db00617882933fbf7dcccacc353abb896c51b Mon Sep 17 00:00:00 2001 From: Hooper Date: Tue, 30 Jun 2026 17:14:06 +0800 Subject: [PATCH 1/3] SDSTOR-22799 feat: chunk ownership tracking, remove GC shard header/footer I/O, blob commit validation - ChunkSelector tracks which shard owns each chunk; select/release enforce ownership and reject wrong-owner calls - GC no longer writes physical shard header/footer blocks - Blob commit validates chunk state matches shard metadata --- conanfile.py | 2 +- src/lib/homestore_backend/gc_manager.cpp | 345 ++++++------------ src/lib/homestore_backend/gc_manager.hpp | 3 - .../homestore_backend/heap_chunk_selector.cpp | 137 ++++--- .../homestore_backend/heap_chunk_selector.h | 36 +- src/lib/homestore_backend/hs_blob_manager.cpp | 51 ++- .../homestore_backend/hs_shard_manager.cpp | 42 ++- .../replication_state_machine.cpp | 2 +- .../snapshot_receive_handler.cpp | 2 +- .../homestore_backend/tests/hs_gc_tests.cpp | 20 +- .../tests/test_heap_chunk_selector.cpp | 144 ++++++-- 11 files changed, 417 insertions(+), 367 deletions(-) diff --git a/conanfile.py b/conanfile.py index 5e03ecdf..2fc86598 100644 --- a/conanfile.py +++ b/conanfile.py @@ -10,7 +10,7 @@ class HomeObjectConan(ConanFile): name = "homeobject" - version = "4.2.2" + version = "4.2.3" homepage = "https://github.com/eBay/HomeObject" description = "Blob Store built on HomeStore" diff --git a/src/lib/homestore_backend/gc_manager.cpp b/src/lib/homestore_backend/gc_manager.cpp index df7baf34..f0eb9577 100644 --- a/src/lib/homestore_backend/gc_manager.cpp +++ b/src/lib/homestore_backend/gc_manager.cpp @@ -650,27 +650,6 @@ bool GCManager::pdev_gc_actor::replace_blob_index( return true; } -sisl::sg_list GCManager::pdev_gc_actor::generate_shard_super_blk_sg_list(shard_id_t shard_id) { - // TODO: do the buffer check before using it. - auto raw_shard_sb = m_hs_home_object->_get_hs_shard(shard_id); - RELEASE_ASSERT(raw_shard_sb, "can not find shard super blk for shard_id=0x{:x} !!!", shard_id); - - const auto shard_sb = - const_cast< HSHomeObject::HS_Shard* >(d_cast< const HSHomeObject::HS_Shard* >(raw_shard_sb))->sb_.get(); - - auto blk_size = homestore::data_service().get_blk_size(); - auto shard_sb_size = sizeof(HSHomeObject::shard_info_superblk); - auto total_size = sisl::round_up(shard_sb_size, blk_size); - auto shard_sb_buf = iomanager.iobuf_alloc(blk_size, total_size); - - std::memcpy(shard_sb_buf, shard_sb, shard_sb_size); - - sisl::sg_list shard_sb_sgs; - shard_sb_sgs.size = total_size; - shard_sb_sgs.iovs.emplace_back(iovec{.iov_base = shard_sb_buf, .iov_len = total_size}); - return shard_sb_sgs; -} - // note that, when we copy data, there is not create shard or put blob in this chunk, only delete blob might happen. bool GCManager::pdev_gc_actor::copy_valid_data( chunk_id_t move_from_chunk, chunk_id_t move_to_chunk, @@ -723,13 +702,11 @@ bool GCManager::pdev_gc_actor::copy_valid_data( homestore::blk_alloc_hints hints; hints.chunk_id_hint = move_to_chunk; - homestore::MultiBlkId out_blkids; auto pg_index_table = m_hs_home_object->get_hs_pg(pg_id)->index_table_; const auto blk_size = data_service.get_blk_size(); for (const auto& shard_id : shards) { - bool is_last_shard = (shard_id == last_shard_id); std::vector< std::pair< BlobRouteKey, BlobRouteValue > > valid_blob_indexes; auto start_key = BlobRouteKey{BlobRoute{shard_id, std::numeric_limits< uint64_t >::min()}}; auto end_key = BlobRouteKey{BlobRoute{shard_id, std::numeric_limits< uint64_t >::max()}}; @@ -786,12 +763,8 @@ bool GCManager::pdev_gc_actor::copy_valid_data( } if (valid_blob_indexes.empty()) { - GCLOGD(task_id, pg_id, shard_id, "empty shard found in move_from_chunk={}", move_from_chunk); - // TODO::send a delete shard request to raft channel. there is a case that when we are doing gc, the - // shard becomes empty, need to handle this case - - // we should always write a shard header for the last shard if the state of it is open. - if (last_shard_state != ShardInfo::State::OPEN || !is_last_shard) continue; + GCLOGD(task_id, pg_id, shard_id, "empty shard found in move_from_chunk={}, skip", move_from_chunk); + continue; } else { GCLOGD(task_id, pg_id, shard_id, "{} valid blobs found in move_from_chunk={}", valid_blob_indexes.size(), move_from_chunk); @@ -812,220 +785,120 @@ bool GCManager::pdev_gc_actor::copy_valid_data( } } - // prepare a shard header for this shard in move_to_chunk - sisl::sg_list header_sgs = generate_shard_super_blk_sg_list(shard_id); - - // we ignore the state in shard header blk. we never read a shard header since we don`t know where it is(nor - // record the pba in indextable) -#if 0 - // we now generate shard header from metablk. the shard state in shard header blk should be open, but for sealed - // shard, the state in the generated in-memory header_sgs is sealed. - if (!is_last_shard_in_emergent_chunk) { - // for the sealed shard, the shard state in header should also be open.now, the written header is the same - // as footer except the shard state, so we lost the original header. - r_cast< HSHomeObject::shard_info_superblk* >(header_sgs.iovs[0].iov_base)->info.state = - ShardInfo::State::OPEN; - - // TODO:: get the original header from the move_from_chunk and change the following part if needed. - /* - uint64_t created_time; - uint64_t last_modified_time; - uint64_t available_capacity_bytes; - uint64_t total_capacity_bytes; - */ - } -#endif - // TODO::involve ratelimiter in the following code, where read/write are scheduled. or do we need a central - // ratelimter shared by all components except client io? - const auto succeed_copying_shard = - // 1 write the shard header to move_to_chunk - data_service.async_alloc_write(header_sgs, hints, out_blkids) - .thenValue([this, &hints, &move_to_chunk, &move_from_chunk, &is_last_shard, &shard_id, &blk_size, - &valid_blob_indexes, &data_service, task_id, &last_shard_state, &copied_blobs, pg_id, - header_sgs = std::move(header_sgs)](auto&& err) { - RELEASE_ASSERT(header_sgs.iovs.size() == 1, "header_sgs.iovs.size() should be 1, but not!"); - // shard header occupies one blk - COUNTER_INCREMENT(metrics_, gc_write_blk_count, 1); - iomanager.iobuf_free(reinterpret_cast< uint8_t* >(header_sgs.iovs[0].iov_base)); - if (err) { - GCLOGE(task_id, pg_id, shard_id, - "Failed to write shard header for move_to_chunk={}, err={}, err_category={}, " - "err_message={}", - move_to_chunk, err.value(), err.category().name(), err.message()); - return folly::makeFuture< bool >(false); - } - - if (valid_blob_indexes.empty()) { - RELEASE_ASSERT(is_last_shard && last_shard_state == ShardInfo::State::OPEN, - "find empty shard in move_from_chunk={} " - "but is_emergent is false, shard_id=0x{:x}, pg_id={}", - move_from_chunk, shard_id, pg_id); - return folly::makeFuture< bool >(true); - } + // copy all the valid blobs in the shard from move_from_chunk to move_to_chunk + std::vector< folly::Future< bool > > futs; + for (const auto& [k, v] : valid_blob_indexes) { + // k is shard_id + blob_id, v is multiblk id + auto pba = v.pbas(); + auto total_size = pba.blk_count() * blk_size; + + // buffer for read and write data + sisl::sg_list data_sgs; + data_sgs.size = total_size; + data_sgs.iovs.emplace_back( + iovec{.iov_base = iomanager.iobuf_alloc(blk_size, total_size), .iov_len = total_size}); + + futs.emplace_back(std::move( + // read blob from move_from_chunk + data_service.async_read(pba, data_sgs, total_size) + .thenValue([this, k, &hints, &move_from_chunk, &move_to_chunk, &data_service, task_id, pg_id, + data_sgs = std::move(data_sgs), pba, &copied_blobs](auto&& err) { + COUNTER_INCREMENT(metrics_, gc_read_blk_count, pba.blk_count()); + RELEASE_ASSERT(data_sgs.iovs.size() == 1, "data_sgs.iovs.size() should be 1, but not!"); + + const auto shard_id = k.key().shard; + const auto blob_id = k.key().blob; + + if (err) { + GCLOGE(task_id, pg_id, shard_id, + "Failed to read blob from move_from_chunk={}, blob_id={}, err={}, " + "err_category={}, err_message={}", + move_from_chunk, blob_id, err.value(), err.category().name(), err.message()); + iomanager.iobuf_free(reinterpret_cast< uint8_t* >(data_sgs.iovs[0].iov_base)); + return folly::makeFuture< bool >(false); + } + + GCLOGD(task_id, pg_id, shard_id, + "successfully read blob from move_from_chunk={}, blob_id={}, pba={}", move_from_chunk, + blob_id, pba.to_string()); + + if (m_enable_read_verify) { + // after a blob is deleted at originator, if it receives a fetch_data request of + // this blob, a fake delete_marker blob will be returned to the requester. This + // case happens in incremental resync scenario. when verifying blob, if it is a + // delete_marker, we let it pass the verification in gc scenario so that it will + // not block any gc task. + if (!m_hs_home_object->verify_blob(data_sgs.iovs[0].iov_base, shard_id, blob_id, true)) { + GCLOGE(task_id, pg_id, shard_id, + "blob verification fails for move_from_chunk={}, blob_id={}, pba={}", + move_from_chunk, blob_id, pba.to_string()); + iomanager.iobuf_free(reinterpret_cast< uint8_t* >(data_sgs.iovs[0].iov_base)); + return folly::makeFuture< bool >(false); + } + } + + // write the blob to the move_to_chunk. we do not care about the blob order in a + // shard since we can not guarantee a certain order + homestore::MultiBlkId new_pba; + return data_service.async_alloc_write(data_sgs, hints, new_pba) + .thenValue([this, shard_id, blob_id, new_pba, &move_to_chunk, task_id, pg_id, &copied_blobs, + data_sgs = std::move(data_sgs)](auto&& err) { + COUNTER_INCREMENT(metrics_, gc_write_blk_count, new_pba.blk_count()); + RELEASE_ASSERT(data_sgs.iovs.size() == 1, "data_sgs.iovs.size() should be 1, but not!"); + iomanager.iobuf_free(reinterpret_cast< uint8_t* >(data_sgs.iovs[0].iov_base)); + if (err) { + GCLOGE(task_id, pg_id, shard_id, + "Failed to write blob to move_to_chunk={}, blob_id={}, err={}, " + "err_category={}, err_message={}", + move_to_chunk, blob_id, err.value(), err.category().name(), err.message()); + return false; + } - std::vector< folly::Future< bool > > futs; - - // 2 copy all the valid blobs in the shard from move_from_chunk to move_to_chunk - for (const auto& [k, v] : valid_blob_indexes) { - // k is shard_id + blob_id, v is multiblk id - auto pba = v.pbas(); - auto total_size = pba.blk_count() * blk_size; - - // buffer for read and write data - sisl::sg_list data_sgs; - data_sgs.size = total_size; - data_sgs.iovs.emplace_back( - iovec{.iov_base = iomanager.iobuf_alloc(blk_size, total_size), .iov_len = total_size}); - - futs.emplace_back(std::move( - // read blob from move_from_chunk - data_service.async_read(pba, data_sgs, total_size) - .thenValue([this, k, &hints, &move_from_chunk, &move_to_chunk, &data_service, task_id, - pg_id, data_sgs = std::move(data_sgs), pba, &copied_blobs](auto&& err) { - COUNTER_INCREMENT(metrics_, gc_read_blk_count, pba.blk_count()); - RELEASE_ASSERT(data_sgs.iovs.size() == 1, - "data_sgs.iovs.size() should be 1, but not!"); - - const auto shard_id = k.key().shard; - const auto blob_id = k.key().blob; - - if (err) { - GCLOGE(task_id, pg_id, shard_id, - "Failed to read blob from move_from_chunk={}, blob_id={}, err={}, " - "err_category={}, err_message={}", - move_from_chunk, blob_id, err.value(), err.category().name(), - err.message()); - iomanager.iobuf_free(reinterpret_cast< uint8_t* >(data_sgs.iovs[0].iov_base)); - return folly::makeFuture< bool >(false); - } - - GCLOGD(task_id, pg_id, shard_id, - "successfully read blob from move_from_chunk={}, blob_id={}, pba={}", - move_from_chunk, blob_id, pba.to_string()); - - if (m_enable_read_verify) { - // after a blob is deleted at originator, if it receives a fetch_data request of - // this blob, a fake delete_marker blob will be returned to the requester. This - // case happens in incremental resync scenario. when verifying blob, if it is a - // delete_marker, we let it pass the verification in gc scenario so that it will - // not block any gc task. - if (!m_hs_home_object->verify_blob(data_sgs.iovs[0].iov_base, shard_id, blob_id, - true)) { - GCLOGE(task_id, pg_id, shard_id, - "blob verification fails for move_from_chunk={}, blob_id={}, pba={}", - move_from_chunk, blob_id, pba.to_string()); - iomanager.iobuf_free( - reinterpret_cast< uint8_t* >(data_sgs.iovs[0].iov_base)); - return folly::makeFuture< bool >(false); - } - } - - // write the blob to the move_to_chunk. we do not care about the blob order in a - // shard since we can not guarantee a certain order - homestore::MultiBlkId new_pba; - return data_service.async_alloc_write(data_sgs, hints, new_pba) - .thenValue([this, shard_id, blob_id, new_pba, &move_to_chunk, task_id, pg_id, - &copied_blobs, data_sgs = std::move(data_sgs)](auto&& err) { - COUNTER_INCREMENT(metrics_, gc_write_blk_count, new_pba.blk_count()); - RELEASE_ASSERT(data_sgs.iovs.size() == 1, - "data_sgs.iovs.size() should be 1, but not!"); - iomanager.iobuf_free( - reinterpret_cast< uint8_t* >(data_sgs.iovs[0].iov_base)); - if (err) { - GCLOGE(task_id, pg_id, shard_id, - "Failed to write blob to move_to_chunk={}, blob_id={}, err={}, " - "err_category={}, err_message={}", - move_to_chunk, blob_id, err.value(), err.category().name(), - err.message()); - return false; - } - - // insert a new entry to gc index table for this blob. [move_to_chunk_id, - // shard_id, blob_id] -> [new pba] - BlobRouteByChunkKey key{BlobRouteByChunk{move_to_chunk, shard_id, blob_id}}; - BlobRouteValue value{new_pba}, existing_value; - - homestore::BtreeSinglePutRequest put_req{ - &key, &value, homestore::btree_put_type::INSERT, &existing_value}; - auto status = m_index_table->put(put_req); - if (status != homestore::btree_status_t::success) { - GCLOGE(task_id, pg_id, shard_id, - "Failed to insert new key to gc index table for " - "move_to_chunk={}, blob_id={}, err={}", - move_to_chunk, blob_id, status); - return false; - } - - GCLOGD(task_id, pg_id, shard_id, - "successfully insert new key to gc index table for " - "move_to_chunk={}, blob_id={}, new_pba={}", - move_to_chunk, blob_id, new_pba.to_string()); - - BlobRouteByChunk route_key{move_to_chunk, shard_id, blob_id}; - auto ret = copied_blobs.insert(route_key, value); - RELEASE_ASSERT(ret.second, - "we should not copy the same blob twice in gc task, " - "move_to_chunk={}, shard_id=0x{:x}, pg_id={}, blob_id={}", - move_to_chunk, shard_id, pg_id, blob_id); - - return true; - }); - }))); - } + // insert a new entry to gc index table for this blob. [move_to_chunk_id, + // shard_id, blob_id] -> [new pba] + BlobRouteByChunkKey key{BlobRouteByChunk{move_to_chunk, shard_id, blob_id}}; + BlobRouteValue value{new_pba}, existing_value; - // 3 write a shard footer for this shard - sisl::sg_list footer_sgs = generate_shard_super_blk_sg_list(shard_id); - return folly::collectAllUnsafe(futs) - .thenValue([this, &is_last_shard, &shard_id, &blk_size, &hints, &move_to_chunk, pg_id, - &last_shard_state, task_id, &data_service, footer_sgs](auto&& results) { - // if any blob copy fails, we will not write footer, and drop this gc task - for (auto const& ok : results) { - RELEASE_ASSERT(ok.hasValue(), "we never throw any exception when copying data"); - if (!ok.value()) { + homestore::BtreeSinglePutRequest put_req{ + &key, &value, homestore::btree_put_type::INSERT, &existing_value}; + auto status = m_index_table->put(put_req); + if (status != homestore::btree_status_t::success) { GCLOGE(task_id, pg_id, shard_id, - "Failed to copy blob for move_to_chunk={}, will cancel this task", - move_to_chunk); - // if any op fails, we drop this gc task. - return folly::makeFuture< std::error_code >( - std::make_error_code(std::errc::operation_canceled)); + "Failed to insert new key to gc index table for " + "move_to_chunk={}, blob_id={}, err={}", + move_to_chunk, blob_id, status); + return false; } - } - // we skip writing footer only if the last shard of this chunk is in open state. - if (is_last_shard && last_shard_state == ShardInfo::State::OPEN) { GCLOGD(task_id, pg_id, shard_id, - "skip writing the footer for move_to_chunk={} for emergent gc task", - move_to_chunk); - return folly::makeFuture< std::error_code >(std::error_code{}); - } + "successfully insert new key to gc index table for " + "move_to_chunk={}, blob_id={}, new_pba={}", + move_to_chunk, blob_id, new_pba.to_string()); + + BlobRouteByChunk route_key{move_to_chunk, shard_id, blob_id}; + auto ret = copied_blobs.insert(route_key, value); + RELEASE_ASSERT(ret.second, + "we should not copy the same blob twice in gc task, " + "move_to_chunk={}, shard_id=0x{:x}, pg_id={}, blob_id={}", + move_to_chunk, shard_id, pg_id, blob_id); + + return true; + }); + }))); + } - // write shard footer, which occupies one blk - - // in fact , we have a dangling reference of out_blkids here, but it does not matter since - // we do not use it after this and the async_alloc_write will complete soon and the memory - // of out_blkids will be released after that. - homestore::MultiBlkId out_blkids; - COUNTER_INCREMENT(metrics_, gc_write_blk_count, 1); - return data_service.async_alloc_write(footer_sgs, hints, out_blkids); - }) - .thenValue([this, &move_to_chunk, &shard_id, footer_sgs, task_id, pg_id](auto&& err) { - RELEASE_ASSERT(footer_sgs.iovs.size() == 1, "footer_sgs.iovs.size() should be 1, but not!"); - iomanager.iobuf_free(reinterpret_cast< uint8_t* >(footer_sgs.iovs[0].iov_base)); - - if (err) { - // we will come here if: - // 1 any blob copy fails, then err is operation_cancelled - // 2 write footer fails， then err is the error code of write footer - GCLOGE( - task_id, pg_id, shard_id, - "Failed to copy some blobs or failed to write shard footer for move_to_chunk={}, " - "err={}, error_category={}, error_message={}, pls check the log for more " - "detailed info", - move_to_chunk, err.value(), err.category().name(), err.message()); - return false; - } - return true; - }); + const auto succeed_copying_shard = + folly::collectAllUnsafe(futs) + .thenValue([this, &shard_id, &move_to_chunk, task_id, pg_id](auto&& results) { + for (auto const& ok : results) { + RELEASE_ASSERT(ok.hasValue(), "we never throw any exception when copying data"); + if (!ok.value()) { + GCLOGE(task_id, pg_id, shard_id, + "Failed to copy blob for move_to_chunk={}, will cancel this task", move_to_chunk); + return false; + } + } + return true; }) .get(); diff --git a/src/lib/homestore_backend/gc_manager.hpp b/src/lib/homestore_backend/gc_manager.hpp index f85d9718..1ee48f49 100644 --- a/src/lib/homestore_backend/gc_manager.hpp +++ b/src/lib/homestore_backend/gc_manager.hpp @@ -292,9 +292,6 @@ class GCManager { pdev_gc_metrics& metrics() { return metrics_; } private: - // utils - sisl::sg_list generate_shard_super_blk_sg_list(shard_id_t shard_id); - private: friend class gc_task_guard; uint32_t m_pdev_id; diff --git a/src/lib/homestore_backend/heap_chunk_selector.cpp b/src/lib/homestore_backend/heap_chunk_selector.cpp index 49f02d34..8a0b0e6b 100644 --- a/src/lib/homestore_backend/heap_chunk_selector.cpp +++ b/src/lib/homestore_backend/heap_chunk_selector.cpp @@ -44,34 +44,10 @@ void HeapChunkSelector::add_chunk_internal(const chunk_num_t p_chunk_id, bool ad } } -// select_chunk will only be called in homestore when creating a shard. +// select_chunk will never be called in homestore since create shard is log only. csharedChunk HeapChunkSelector::select_chunk(homestore::blk_count_t count, const homestore::blk_alloc_hints& hint) { - auto& chunkIdHint = hint.chunk_id_hint; - if (chunkIdHint.has_value()) { - LOGWARNMOD(homeobject, "should not allocated a chunk with exiting chunkIdHint={} in hint!", - chunkIdHint.value()); - return nullptr; - } - - if (!hint.application_hint.has_value()) { - LOGWARNMOD(homeobject, "should not allocated a chunk without exiting application_hint in hint!"); - return nullptr; - } else { - // Both chunk_num_t and pg_id_t are of type uint16_t. - static_assert(std::is_same< pg_id_t, uint16_t >::value, "pg_id_t is not uint16_t"); - static_assert(std::is_same< homestore::chunk_num_t, uint16_t >::value, "chunk_num_t is not uint16_t"); - auto application_hint = hint.application_hint.value(); - pg_id_t pg_id = (uint16_t)(application_hint >> 16 & 0xFFFF); - homestore::chunk_num_t v_chunk_id = (uint16_t)(application_hint & 0xFFFF); - auto exVChunk = select_specific_chunk(pg_id, v_chunk_id); - if (exVChunk == nullptr) { - LOGWARNMOD(homeobject, "failed to select chunk with pg_id={} and v_chunk_id={} from application_hint={}", - pg_id, v_chunk_id, application_hint); - return nullptr; - } else { - return exVChunk->get_internal_chunk(); - } - } + RELEASE_ASSERT(false, "create shard is log only, this function should never be called"); + return nullptr; } bool HeapChunkSelector::try_mark_chunk_to_gc_state(const chunk_num_t chunk_id, bool force) { @@ -114,9 +90,9 @@ void HeapChunkSelector::mark_chunk_out_of_gc_state(const chunk_num_t chunk_id, c } homestore::cshared< HeapChunkSelector::ExtendedVChunk > -HeapChunkSelector::select_specific_chunk(const pg_id_t pg_id, const chunk_num_t v_chunk_id) { +HeapChunkSelector::select_specific_chunk(const pg_id_t pg_id, const chunk_num_t v_chunk_id, + const shard_id_t owner_shard_id) { homestore::shared< ExtendedVChunk > chunk; - while (true) { { std::unique_lock lock_guard(m_chunk_selector_mtx); @@ -125,7 +101,6 @@ HeapChunkSelector::select_specific_chunk(const pg_id_t pg_id, const chunk_num_t LOGWARNMOD(homeobject, "No pg found for pg={}", pg_id); return nullptr; } - auto pg_chunk_collection = pg_it->second; auto& pg_chunks = pg_chunk_collection->m_pg_chunks; std::scoped_lock lock(pg_chunk_collection->mtx); @@ -133,28 +108,36 @@ HeapChunkSelector::select_specific_chunk(const pg_id_t pg_id, const chunk_num_t LOGWARNMOD(homeobject, "No chunk found for v_chunk_id={}", v_chunk_id); return nullptr; } - chunk = pg_chunks[v_chunk_id]; - - if (chunk->m_state == ChunkState::GC) { + if (!chunk->valid()) { + LOGERRORMOD( + homeobject, "v_chunk_id={} pg={} invalid state before select: state={} owner=0x{:x} pchunk={}", + v_chunk_id, pg_id, chunk->m_state, chunk->m_owner_shard_id.value_or(0), chunk->get_chunk_id()); + return nullptr; + } else if (chunk->m_state == ChunkState::GC) { LOGDEBUGMOD(homeobject, "v_chunk_id={} for pg={} is pchunk_id={}, in GC state, wait and retry!", v_chunk_id, pg_id, chunk->get_chunk_id()); - } else { - if (chunk->m_state == ChunkState::AVAILABLE) { - chunk->m_state = ChunkState::INUSE; - --pg_chunk_collection->available_num_chunks; - pg_chunk_collection->available_blk_count -= chunk->available_blks(); - } + } else if (chunk->m_state == ChunkState::AVAILABLE) { + chunk->m_state = ChunkState::INUSE; + chunk->m_owner_shard_id = owner_shard_id; + RELEASE_ASSERT( + chunk->valid(), + "v_chunk_id={} pg={} invalid state after AVAILABLE→INUSE: state={} owner=0x{:x} pchunk={}", + v_chunk_id, pg_id, chunk->m_state, chunk->m_owner_shard_id.value_or(0), chunk->get_chunk_id()); + --pg_chunk_collection->available_num_chunks; + pg_chunk_collection->available_blk_count -= chunk->available_blks(); break; + } else { + // INUSE with valid owner (guaranteed by valid()): same owner → idempotent; other owner → error. + if (chunk->m_owner_shard_id == owner_shard_id) { break; } + LOGWARNMOD(homeobject, + "v_chunk_id={} for pg={} is INUSE by shard=0x{:x}, cannot select for shard=0x{:x}", + v_chunk_id, pg_id, chunk->m_owner_shard_id.value_or(0), owner_shard_id); + return nullptr; } } - - // if the chunk is not available, probably being gc. we wait for a while and retry std::this_thread::sleep_for(std::chrono::seconds(1)); } - - LOGDEBUGMOD(homeobject, "chunk={} is selected for v_chunk_id={}, pg={}", chunk->get_chunk_id(), v_chunk_id, pg_id); - return chunk; } @@ -164,14 +147,14 @@ void HeapChunkSelector::foreach_chunks(std::function< void(csharedChunk&) >&& cb [cb = std::move(cb)](auto& p) { cb(p.second->get_internal_chunk()); }); } -bool HeapChunkSelector::release_chunk(const pg_id_t pg_id, const chunk_num_t v_chunk_id) { +bool HeapChunkSelector::release_chunk(const pg_id_t pg_id, const chunk_num_t v_chunk_id, + const shard_id_t owner_shard_id) { std::unique_lock lock_guard(m_chunk_selector_mtx); auto pg_it = m_per_pg_chunks.find(pg_id); if (pg_it == m_per_pg_chunks.end()) { LOGWARNMOD(homeobject, "No pg found for pg={}", pg_id); return false; } - auto pg_chunk_collection = pg_it->second; auto& pg_chunks = pg_chunk_collection->m_pg_chunks; if (v_chunk_id >= pg_chunks.size()) { @@ -180,8 +163,25 @@ bool HeapChunkSelector::release_chunk(const pg_id_t pg_id, const chunk_num_t v_c } std::scoped_lock lock(pg_chunk_collection->mtx); auto chunk = pg_chunks[v_chunk_id]; + if (!chunk->valid()) { + LOGERRORMOD(homeobject, "v_chunk_id={} pg={} invalid state before release: state={} owner=0x{:x} pchunk={}", + v_chunk_id, pg_id, chunk->m_state, chunk->m_owner_shard_id.value_or(0), chunk->get_chunk_id()); + return false; + } + if (chunk->m_state == ChunkState::AVAILABLE) { + return true; // idempotent for replay + } if (chunk->m_state == ChunkState::INUSE) { + if (chunk->m_owner_shard_id != owner_shard_id) { + LOGWARNMOD(homeobject, "v_chunk_id={} for pg={} is owned by shard=0x{:x}, release by shard=0x{:x} rejected", + v_chunk_id, pg_id, chunk->m_owner_shard_id.value_or(0), owner_shard_id); + return false; + } chunk->m_state = ChunkState::AVAILABLE; + chunk->m_owner_shard_id.reset(); + RELEASE_ASSERT(chunk->valid(), + "v_chunk_id={} pg={} invalid state after INUSE→AVAILABLE: state={} owner=0x{:x} pchunk={}", + v_chunk_id, pg_id, chunk->m_state, chunk->m_owner_shard_id.value_or(0), chunk->get_chunk_id()); ++pg_chunk_collection->available_num_chunks; pg_chunk_collection->available_blk_count += chunk->available_blks(); } @@ -228,6 +228,7 @@ bool HeapChunkSelector::return_pg_chunks_to_dev_heap(const pg_id_t pg_id) { } // with shard which should be first chunk->m_pg_id = std::nullopt; chunk->m_v_chunk_id = std::nullopt; + chunk->m_owner_shard_id = std::nullopt; pdev_heap->m_heap.emplace(chunk); pdev_heap->available_blk_count += chunk->available_blks(); @@ -347,10 +348,27 @@ void HeapChunkSelector::update_vchunk_info_after_gc(const chunk_num_t move_from_ // 1 change the pg_id and vchunk_id of the move_to_chunk according to metablk move_to_vchunk->m_pg_id = pg_id; move_to_vchunk->m_v_chunk_id = vchunk_id; + if (final_state == ChunkState::INUSE) { + // EGC: move_to takes over as the active chunk for the open shard and must carry its owner_shard_id. + // + // Two cases: + // Normal EGC (first run): move_to was a reserved chunk with no owner; copy owner from move_from. + // Crash-recovery replay: after crash, pg_superblk and shard_superblk already point to move_to, + // so recover_pg_chunks_states() has already set move_to.owner correctly. move_from is no + // longer listed in pg_superblk, so recover_pg_chunks_states() leaves its owner = nullopt. + // Copying from move_from would overwrite the correct owner with null — skip the copy. + if (!move_to_vchunk->m_owner_shard_id.has_value()) { + move_to_vchunk->m_owner_shard_id = move_from_vchunk->m_owner_shard_id; + } + } else { + // Normal GC: move_to becomes AVAILABLE for future shards in this PG and must have no owner. + move_to_vchunk->m_owner_shard_id = std::nullopt; + } // 2 update the chunk state of move_from_chunk, now it is a reserved chunk move_from_vchunk->m_pg_id = std::nullopt; move_from_vchunk->m_v_chunk_id = std::nullopt; + move_from_vchunk->m_owner_shard_id = std::nullopt; // 3 update the state of move_to_chunk, so that it can be used for creating shard or putting blob. we need to do // this after reserved_chunk meta blk is updated, so that if crash happens, we recovery the move_to_chunk is the @@ -472,7 +490,7 @@ void HeapChunkSelector::build_pdev_available_chunk_heap() { } bool HeapChunkSelector::recover_pg_chunks_states(pg_id_t pg_id, - const std::unordered_set< chunk_num_t >& excluding_v_chunk_ids) { + const std::unordered_map< chunk_num_t, shard_id_t >& inuse_chunks) { std::unique_lock lock_guard(m_chunk_selector_mtx); auto pg_it = m_per_pg_chunks.find(pg_id); if (pg_it == m_per_pg_chunks.end()) { @@ -487,13 +505,15 @@ bool HeapChunkSelector::recover_pg_chunks_states(pg_id_t pg_id, for (size_t v_chunk_id = 0; v_chunk_id < pg_chunks.size(); ++v_chunk_id) { auto chunk = pg_chunks[v_chunk_id]; pg_chunk_collection->m_total_blks += chunk->get_total_blks(); - if (excluding_v_chunk_ids.find(v_chunk_id) == excluding_v_chunk_ids.end()) { + auto it = inuse_chunks.find(v_chunk_id); + if (it == inuse_chunks.end()) { chunk->m_state = ChunkState::AVAILABLE; ++pg_chunk_collection->available_num_chunks; pg_chunk_collection->available_blk_count += chunk->available_blks(); } else { chunk->m_state = ChunkState::INUSE; + chunk->m_owner_shard_id = it->second; } } return true; @@ -519,7 +539,7 @@ std::shared_ptr< const std::vector< homestore::chunk_num_t > > HeapChunkSelector } homestore::cshared< HeapChunkSelector::ExtendedVChunk > -HeapChunkSelector::pick_most_available_blk_chunk(uint64_t ctx, pg_id_t pg_id) { +HeapChunkSelector::pick_most_available_blk_chunk(shard_id_t shard_id, pg_id_t pg_id) { std::shared_lock lock_guard(m_chunk_selector_mtx); auto pg_it = m_per_pg_chunks.find(pg_id); if (pg_it == m_per_pg_chunks.end()) { @@ -535,16 +555,23 @@ HeapChunkSelector::pick_most_available_blk_chunk(uint64_t ctx, pg_id_t pg_id) { return !a->available() || (b->available() && a->available_blks() < b->available_blks()); }); if (!(*max_it)->available()) { - LOGWARNMOD(homeobject, "No available chunk for pg={}, ctx=0x{:x}", pg_id, ctx); + LOGWARNMOD(homeobject, "No available chunk for pg={}, shard_id=0x{:x}", pg_id, shard_id); return nullptr; } auto v_chunk_id = std::distance(pg_chunks.begin(), max_it); - LOGDEBUGMOD(homeobject, "Picked v_chunk_id={} : [p_chunk_id={}, avail={}], ctx=0x{:x}", v_chunk_id, - pg_chunks[v_chunk_id]->get_chunk_id(), pg_chunks[v_chunk_id]->available_blks(), ctx); - pg_chunks[v_chunk_id]->m_state = ChunkState::INUSE; + auto& picked = pg_chunks[v_chunk_id]; + RELEASE_ASSERT(picked->valid(), "v_chunk_id={} pg={} invalid state before pick: state={} owner=0x{:x} pchunk={}", + v_chunk_id, pg_id, picked->m_state, picked->m_owner_shard_id.value_or(0), picked->get_chunk_id()); + LOGDEBUGMOD(homeobject, "Picked v_chunk_id={} : [p_chunk_id={}, avail={}], shard_id=0x{:x}", v_chunk_id, + picked->get_chunk_id(), picked->available_blks(), shard_id); + picked->m_state = ChunkState::INUSE; + picked->m_owner_shard_id = shard_id; + RELEASE_ASSERT(picked->valid(), + "v_chunk_id={} pg={} invalid state after AVAILABLE→INUSE in pick: state={} owner=0x{:x} pchunk={}", + v_chunk_id, pg_id, picked->m_state, picked->m_owner_shard_id.value_or(0), picked->get_chunk_id()); --pg_chunk_collection->available_num_chunks; - pg_chunk_collection->available_blk_count -= pg_chunks[v_chunk_id]->available_blks(); - return pg_chunks[v_chunk_id]; + pg_chunk_collection->available_blk_count -= picked->available_blks(); + return picked; } // return the maximum number of chunks that can be allocated on pdev diff --git a/src/lib/homestore_backend/heap_chunk_selector.h b/src/lib/homestore_backend/heap_chunk_selector.h index 1ffa12c1..cc032e88 100644 --- a/src/lib/homestore_backend/heap_chunk_selector.h +++ b/src/lib/homestore_backend/heap_chunk_selector.h @@ -32,12 +32,25 @@ class HeapChunkSelector : public homestore::ChunkSelector { class ExtendedVChunk : public VChunk { public: ExtendedVChunk(csharedChunk const& chunk) : - VChunk(chunk), m_state(ChunkState::AVAILABLE), m_pg_id(), m_v_chunk_id() {} + VChunk(chunk), m_state(ChunkState::AVAILABLE), m_pg_id(), m_v_chunk_id(), m_owner_shard_id() {} ~ExtendedVChunk() = default; ChunkState m_state; std::optional< pg_id_t > m_pg_id; std::optional< chunk_num_t > m_v_chunk_id; - bool available() const { return m_state == ChunkState::AVAILABLE; } + std::optional< shard_id_t > m_owner_shard_id; // runtime only, not persisted + bool available() const { return valid() && m_state == ChunkState::AVAILABLE; } + bool valid() const { + switch (m_state) { + case ChunkState::AVAILABLE: + return !m_owner_shard_id.has_value(); + case ChunkState::INUSE: + return m_owner_shard_id.has_value(); + case ChunkState::GC: + return true; + default: + return false; + } + } }; class ExtendedVChunkComparator { @@ -73,9 +86,10 @@ class HeapChunkSelector : public homestore::ChunkSelector { csharedChunk select_chunk([[maybe_unused]] homestore::blk_count_t nblks, const homestore::blk_alloc_hints& hints); - // this function will be used by create shard or recovery flow to mark one specific chunk to be busy, caller should - // be responsible to use release_chunk() interface to release it when no longer to use the chunk anymore. - homestore::cshared< ExtendedVChunk > select_specific_chunk(const pg_id_t pg_id, const chunk_num_t v_chunk_id); + // State machine: GC→wait+retry, AVAILABLE→INUSE(owner=shard), INUSE+same_owner→success, INUSE+other_owner→fail. + // Only leader will call this function twice during shard creation, follower will only call once. + homestore::cshared< ExtendedVChunk > select_specific_chunk(const pg_id_t pg_id, const chunk_num_t v_chunk_id, + const shard_id_t owner_shard_id); /** * try to mark a chunk as gc state, so that it will not be selected by any creating shard. @@ -89,9 +103,9 @@ class HeapChunkSelector : public homestore::ChunkSelector { void mark_chunk_out_of_gc_state(const chunk_num_t chunk_id, const ChunkState final_state, const uint64_t task_id); - // This function returns a chunk back to ChunkSelector. - // It is used in two scenarios: 1. seal shard 2. create shard rollback - bool release_chunk(const pg_id_t pg_id, const chunk_num_t v_chunk_id); + // Release a vchunk back to AVAILABLE. Idempotent if already AVAILABLE (safe for replay). + // INUSE + wrong owner → logs warning, returns false. + bool release_chunk(const pg_id_t pg_id, const chunk_num_t v_chunk_id, const shard_id_t owner_shard_id); bool reset_pg_chunks(pg_id_t pg_id); @@ -122,11 +136,11 @@ class HeapChunkSelector : public homestore::ChunkSelector { /** * pop pg top chunk * - * @param ctx only for logging. + * @param shard_id select chunk for this shard * @param pg_id The ID of the pg. * @return The most available ExtendedVChunk for the pg, or nullptr if no chunk is available. */ - homestore::cshared< ExtendedVChunk > pick_most_available_blk_chunk(uint64_t ctx, pg_id_t pg_id); + homestore::cshared< ExtendedVChunk > pick_most_available_blk_chunk(shard_id_t shard_id, pg_id_t pg_id); // this should be called on each pg meta blk found bool recover_pg_chunks(pg_id_t pg_id, std::vector< chunk_num_t >&& p_chunk_ids); @@ -135,7 +149,7 @@ class HeapChunkSelector : public homestore::ChunkSelector { void build_pdev_available_chunk_heap(); // this should be called after ShardManager is initialized and get all the open shards - bool recover_pg_chunks_states(pg_id_t pg_id, const std::unordered_set< chunk_num_t >& excluding_v_chunk_ids); + bool recover_pg_chunks_states(pg_id_t pg_id, const std::unordered_map< chunk_num_t, shard_id_t >& inuse_chunks); /** * Returns the number of available blocks of the given device id. diff --git a/src/lib/homestore_backend/hs_blob_manager.cpp b/src/lib/homestore_backend/hs_blob_manager.cpp index 575f7c11..2ca0b7bc 100644 --- a/src/lib/homestore_backend/hs_blob_manager.cpp +++ b/src/lib/homestore_backend/hs_blob_manager.cpp @@ -229,6 +229,53 @@ bool HSHomeObject::local_add_blob_info(pg_id_t const pg_id, BlobInfo const& blob return false; } if (!exist_already) { + // check the shard's vchunk and pchunk id for this blob, and validate the chunk state at commit time. + // For log replay, since seal shard will trigger cp flush, so there only will be one shard's log related to one + // specific chunk. GC will not be triggered for this chunk, so we can safely check the chunk state at commit + // time. + homestore::chunk_num_t sb_vchunk_id; + homestore::chunk_num_t sb_pchunk_id; + { + std::shared_lock lock_guard(_shard_lock); + const auto iter = _shard_map.find(blob_info.shard_id); + RELEASE_ASSERT(iter != _shard_map.end(), "shardID=0x{:x}, pg={}, shard=0x{:x}, shard does not exist", + blob_info.shard_id, (blob_info.shard_id >> homeobject::shard_width), + (blob_info.shard_id & homeobject::shard_mask)); + const auto* hs_shard = d_cast< HS_Shard* >((*iter->second).get()); + sb_vchunk_id = hs_shard->v_chunk_id(); + sb_pchunk_id = hs_shard->p_chunk_id(); + } + // check the blob's pchunk id and shard's pchunk id for this blob + if (blob_info.pbas.chunk_num() != sb_pchunk_id) { + BLOGE(tid, blob_info.shard_id, blob_info.blob_id, + "commit-time pchunk id mismatch: blob pchunk={} shard pchunk={}", blob_info.pbas.chunk_num(), + sb_pchunk_id); + RELEASE_ASSERT(false, "traceID={}, commit-time pchunk id mismatch: blob pchunk={} shard pchunk={}", tid, + blob_info.pbas.chunk_num(), sb_pchunk_id); + } + // check shard meta consistency + const auto ext_chunk = chunk_selector()->get_pg_vchunk(pg_id, sb_vchunk_id); + if (ext_chunk == nullptr) { + BLOGE(tid, blob_info.shard_id, blob_info.blob_id, + "chunk not found at commit time: pg={} v_chunk={} shard=0x{:x} blob={}", pg_id, sb_vchunk_id, + blob_info.shard_id, blob_info.blob_id); + RELEASE_ASSERT(false, "traceID={}, chunk not found at commit time: pg={} v_chunk={} shard=0x{:x} blob={}", + tid, pg_id, sb_vchunk_id, blob_info.shard_id, blob_info.blob_id); + } else if (!ext_chunk->valid() || ext_chunk->m_state != ChunkState::INUSE || + ext_chunk->m_owner_shard_id != blob_info.shard_id || ext_chunk->get_chunk_id() != sb_pchunk_id) { + BLOGE(tid, blob_info.shard_id, blob_info.blob_id, + "commit-time chunk state invalid: pg={} v_chunk={} blob={} state={} owner=0x{:x} pchunk={} " + "sb_pchunk={}", + pg_id, sb_vchunk_id, blob_info.blob_id, ext_chunk->m_state, ext_chunk->m_owner_shard_id.value_or(0), + ext_chunk->get_chunk_id(), sb_pchunk_id); + RELEASE_ASSERT( + false, + "traceID={}, commit-time chunk state invalid: pg={} v_chunk={} blob={} state={} owner=0x{:x} " + "pchunk={} sb_pchunk={}", + tid, pg_id, sb_vchunk_id, blob_info.blob_id, ext_chunk->m_state, + ext_chunk->m_owner_shard_id.value_or(0), ext_chunk->get_chunk_id(), sb_pchunk_id); + } + // The PG superblock (durable entities) will be persisted as part of HS_CLIENT Checkpoint, which is always // done ahead of the Index Checkpoint. Hence, if the index already has this entity, whatever durable // counters updated as part of the update would have been persisted already in PG superblock. So if we were @@ -280,8 +327,8 @@ void HSHomeObject::on_blob_put_commit(int64_t lsn, sisl::blob const& header, sis int64_t shard_sealed_lsn; { - std::scoped_lock lock_guard(_shard_lock); - auto iter = _shard_map.find(shard_id); + std::shared_lock lock_guard(_shard_lock); + const auto iter = _shard_map.find(shard_id); RELEASE_ASSERT(iter != _shard_map.end(), "shardID=0x{:x}, pg={}, shard=0x{:x}, shard does not exist", shard_id, (shard_id >> homeobject::shard_width), (shard_id & homeobject::shard_mask)); shard_sealed_lsn = (*iter->second)->info.sealed_lsn; diff --git a/src/lib/homestore_backend/hs_shard_manager.cpp b/src/lib/homestore_backend/hs_shard_manager.cpp index 874ad27e..1d3d2f56 100644 --- a/src/lib/homestore_backend/hs_shard_manager.cpp +++ b/src/lib/homestore_backend/hs_shard_manager.cpp @@ -218,7 +218,7 @@ ShardManager::AsyncResult< ShardInfo > HSHomeObject::_create_shard(pg_id_t pg_ow "left space", pg_owner, pchunk_id, v_chunk_id); - bool res = chunk_selector()->release_chunk(pg_owner, v_chunk_id); + bool res = chunk_selector()->release_chunk(pg_owner, v_chunk_id, new_shard_id); RELEASE_ASSERT(res, "Failed to release v_chunk_id={}, pg={}", v_chunk_id, pg_owner); gc_manager()->submit_gc_task(task_priority::normal, pchunk_id); @@ -274,7 +274,7 @@ ShardManager::AsyncResult< ShardInfo > HSHomeObject::_create_shard(pg_id_t pg_ow if (err.getCode() == ShardErrorCode::NOT_LEADER) { err.current_leader = repl_dev->get_leader_id(); } // we will never get no_space_left error here. - bool res = chunk_selector()->release_chunk(pg_owner, v_chunk_id); + bool res = chunk_selector()->release_chunk(pg_owner, v_chunk_id, new_shard_id); RELEASE_ASSERT(res, "Failed to release v_chunk_id={}, pg={}", v_chunk_id, pg_owner); SLOGE(tid, new_shard_id, "got {} when creating shard at leader, failed to create shard {}!", err.getCode(), @@ -502,7 +502,7 @@ void HSHomeObject::local_create_shard(ShardInfo shard_info, homestore::chunk_num // select_specific_chunk() will do something only when we are relaying journal after restart, during the // runtime flow chunk is already been be mark busy when we write the shard info to the repldev. const auto pg_id = shard_info.placement_group; - auto chunk = chunk_selector_->select_specific_chunk(pg_id, v_chunk_id); + auto chunk = chunk_selector_->select_specific_chunk(pg_id, v_chunk_id, shard_info.id); RELEASE_ASSERT(chunk != nullptr, "chunk selection failed with v_chunk_id={} in pg={}", v_chunk_id, pg_id); // we need to add shard to map after chunk is marked in_use. Otherwise, there is a corner case that put_blob @@ -569,8 +569,8 @@ void HSHomeObject::on_shard_message_commit(int64_t lsn, sisl::blob const& h, sha ShardInfo::State state; { - std::scoped_lock lock_guard(_shard_lock); - auto iter = _shard_map.find(shard_info.id); + std::shared_lock lock_guard(_shard_lock); + const auto iter = _shard_map.find(shard_info.id); RELEASE_ASSERT(iter != _shard_map.end(), "shardID=0x{:x}, pg={}, shard=0x{:x}, shard does not exist", shard_info.id, (shard_info.id >> homeobject::shard_width), (shard_info.id & homeobject::shard_mask)); @@ -610,7 +610,7 @@ void HSHomeObject::on_shard_message_commit(int64_t lsn, sisl::blob const& h, sha auto pg_id = shard_info.placement_group; auto v_chunkID = get_shard_v_chunk_id(shard_info.id); RELEASE_ASSERT(v_chunkID.has_value(), "v_chunk id not found"); - bool res = chunk_selector()->release_chunk(pg_id, v_chunkID.value()); + bool res = chunk_selector()->release_chunk(pg_id, v_chunkID.value(), shard_info.id); RELEASE_ASSERT(res, "Failed to release v_chunk_id={}, pg={}", v_chunkID.value(), pg_id); if (ctx) { ctx->promise_.setValue(ShardManager::Result< ShardInfo >(shard_info)); } @@ -662,21 +662,23 @@ void HSHomeObject::on_shard_meta_blk_found(homestore::meta_blk* mblk, sisl::byte } void HSHomeObject::on_shard_meta_blk_recover_completed(bool success) { - std::unordered_set< homestore::chunk_num_t > excluding_chunks; + std::unordered_map< homestore::chunk_num_t, shard_id_t > inuse_chunks; std::scoped_lock lock_guard(_pg_lock); for (auto& pair : _pg_map) { if (dynamic_cast< HS_PG* >(pair.second.get())->pg_state_.is_state_set(PGStateMask::DISK_DOWN)) { LOGW("pg={} is disk down, skip recover chunk state from shards", pair.first); continue; } - excluding_chunks.clear(); - excluding_chunks.reserve(pair.second->shards_.size()); + inuse_chunks.clear(); + inuse_chunks.reserve(pair.second->shards_.size()); for (auto& shard : pair.second->shards_) { if (shard->info.state == ShardInfo::State::OPEN) { - excluding_chunks.emplace(d_cast< HS_Shard* >(shard.get())->sb_->v_chunk_id); + inuse_chunks.emplace(d_cast< HS_Shard* >(shard.get())->sb_->v_chunk_id, shard->info.id); } } - bool res = chunk_selector_->recover_pg_chunks_states(pair.first, excluding_chunks); + // After restart, each chunk retains at most one shard log, because every + // seal-shard commit forces a cp flush. + bool res = chunk_selector_->recover_pg_chunks_states(pair.first, inuse_chunks); RELEASE_ASSERT(res, "Failed to recover pg chunk heap, pg={}", pair.first); } } @@ -754,22 +756,22 @@ void HSHomeObject::update_shard_in_map(const ShardInfo& shard_info) { } const Shard* HSHomeObject::_get_hs_shard(const shard_id_t shard_id) const { - std::scoped_lock lock_guard(_shard_lock); - auto shard_iter = _shard_map.find(shard_id); + std::shared_lock lock_guard(_shard_lock); + const auto shard_iter = _shard_map.find(shard_id); if (shard_iter == _shard_map.end()) { return nullptr; } return (*shard_iter->second).get(); } std::optional< homestore::chunk_num_t > HSHomeObject::get_shard_p_chunk_id(shard_id_t id) const { - std::scoped_lock lock_guard(_shard_lock); - auto shard_iter = _shard_map.find(id); + std::shared_lock lock_guard(_shard_lock); + const auto shard_iter = _shard_map.find(id); if (shard_iter == _shard_map.end()) { return std::nullopt; } - auto hs_shard = d_cast< HS_Shard* >((*shard_iter->second).get()); + const auto* hs_shard = d_cast< const HS_Shard* >((*shard_iter->second).get()); return std::make_optional< homestore::chunk_num_t >(hs_shard->sb_->p_chunk_id); } const std::set< shard_id_t > HSHomeObject::get_shards_in_chunk(homestore::chunk_num_t chunk_id) const { - std::scoped_lock lock_guard(_shard_lock); + std::shared_lock lock_guard(_shard_lock); const auto it = chunk_to_shards_map_.find(chunk_id); if (it == chunk_to_shards_map_.cend()) { LOGW("chunk_id={} not found in chunk_to_shards_map", chunk_id); @@ -830,10 +832,10 @@ void HSHomeObject::update_shard_meta_after_gc(const homestore::chunk_num_t move_ } std::optional< homestore::chunk_num_t > HSHomeObject::get_shard_v_chunk_id(const shard_id_t id) const { - std::scoped_lock lock_guard(_shard_lock); - auto shard_iter = _shard_map.find(id); + std::shared_lock lock_guard(_shard_lock); + const auto shard_iter = _shard_map.find(id); if (shard_iter == _shard_map.end()) { return std::nullopt; } - auto hs_shard = d_cast< HS_Shard* >((*shard_iter->second).get()); + const auto* hs_shard = d_cast< const HS_Shard* >((*shard_iter->second).get()); return std::make_optional< homestore::chunk_num_t >(hs_shard->sb_->v_chunk_id); } diff --git a/src/lib/homestore_backend/replication_state_machine.cpp b/src/lib/homestore_backend/replication_state_machine.cpp index 838a4373..e2ebc3fa 100644 --- a/src/lib/homestore_backend/replication_state_machine.cpp +++ b/src/lib/homestore_backend/replication_state_machine.cpp @@ -964,7 +964,7 @@ void ReplicationStateMachine::on_log_replay_done(const homestore::group_id_t& gr if (shard_sb->info.is_open()) { const auto pg_id = shard_sb->info.placement_group; const auto vchunk_id = shard_sb->v_chunk_id; - const auto exVchunk = chunk_selector->select_specific_chunk(pg_id, vchunk_id); + const auto exVchunk = chunk_selector->select_specific_chunk(pg_id, vchunk_id, shard_sb->info.id); RELEASE_ASSERT(exVchunk != nullptr, "chunk selection failed with v_chunk_id={} in pg={}", vchunk_id, pg_id); LOGD("vchunk={} is selected for shard={} in pg={} when recovery", vchunk_id, shard_sb->info.id, pg_id); } diff --git a/src/lib/homestore_backend/snapshot_receive_handler.cpp b/src/lib/homestore_backend/snapshot_receive_handler.cpp index bd3ab3be..1b33d3e7 100644 --- a/src/lib/homestore_backend/snapshot_receive_handler.cpp +++ b/src/lib/homestore_backend/snapshot_receive_handler.cpp @@ -301,7 +301,7 @@ int HSHomeObject::SnapshotReceiveHandler::process_blobs_snapshot_data(ResyncBlob state = (*iter->second)->info.state; } if (state == ShardInfo::State::SEALED) { - home_obj_.chunk_selector()->release_chunk(ctx_->pg_id, v_chunk_id.value()); + home_obj_.chunk_selector()->release_chunk(ctx_->pg_id, v_chunk_id.value(), ctx_->shard_cursor); } { std::unique_lock< std::shared_mutex > lock(ctx_->progress_lock); diff --git a/src/lib/homestore_backend/tests/hs_gc_tests.cpp b/src/lib/homestore_backend/tests/hs_gc_tests.cpp index 775ce430..d10c711f 100644 --- a/src/lib/homestore_backend/tests/hs_gc_tests.cpp +++ b/src/lib/homestore_backend/tests/hs_gc_tests.cpp @@ -117,7 +117,7 @@ TEST_F(HomeObjectFixture, BasicGC) { ASSERT_TRUE(chunk_opt.has_value()); auto chunk_id = chunk_opt.value(); // now, the chunk state is not determined, maybe GC(being gc) or AVAILABLE(complete gc), skip checking it. - uint32_t used_blks{2}; /* header and footer */ + uint32_t used_blks{0}; for (const auto& [_, blk_count] : blob_to_blk_count) { used_blks += blk_count; @@ -627,13 +627,10 @@ void HomeObjectFixture::EmergentGC(bool with_crash_recovery) { uint64_t total_blob_occupied_blk_count{0}; const auto& shard_vec = pg_shard_id_vec[pg_id]; for (const auto& shard_id : shard_vec) { - total_blob_occupied_blk_count += 2; /*header and footer*/ for (const auto& [_, blk_count] : shard_blob_ids_map[shard_id]) { total_blob_occupied_blk_count += blk_count; } } - // for each chunk, we have an open shard, which has only header. - total_blob_occupied_blk_count -= pg_chunk_nums[pg_id]; ASSERT_EQ(hs_pg->pg_sb_->total_occupied_blk_count, total_blob_occupied_blk_count); ASSERT_EQ(hs_pg->durable_entities().total_occupied_blk_count, total_blob_occupied_blk_count); @@ -685,13 +682,10 @@ void HomeObjectFixture::EmergentGC(bool with_crash_recovery) { uint64_t total_blob_occupied_blk_count{0}; const auto& shard_vec = pg_shard_id_vec[pg_id]; for (const auto& shard_id : shard_vec) { - total_blob_occupied_blk_count += 2; /*header and footer*/ for (const auto& [_, blk_count] : shard_blob_ids_map[shard_id]) { total_blob_occupied_blk_count += blk_count; } } - // for each chunk, we have an open shard, which has only header. - total_blob_occupied_blk_count -= pg_chunk_nums[pg_id]; ASSERT_EQ(hs_pg->pg_sb_->total_occupied_blk_count, total_blob_occupied_blk_count); ASSERT_EQ(hs_pg->durable_entities().total_occupied_blk_count, total_blob_occupied_blk_count); @@ -728,25 +722,23 @@ void HomeObjectFixture::EmergentGC(bool with_crash_recovery) { futs.clear(); - // for each chunk in this pg, there is only one shard header + // for each chunk in this pg, all blobs are gone (no header/footer since shard create is log-only) for (const auto& [pg_id, chunk_num] : pg_chunk_nums) { const auto pg_chunks = chunk_selector->get_pg_chunks(pg_id); for (uint64_t i{0}; i < chunk_num; i++) { auto chunk_id = pg_chunks->at(i); auto EXVchunk = chunk_selector->get_extend_vchunk(chunk_id); - // the open shard is not sealed, so there is only the shard header for each shard - ASSERT_EQ(EXVchunk->get_used_blks(), 1); + ASSERT_EQ(EXVchunk->get_used_blks(), 0); } } // check vchunk to pchunk for every pg for (const auto& [pg_id, shard_vec] : pg_shard_id_vec) { auto& hs_pg = HS_PG_map[pg_id]; - // check pg durable entities. only shard header left, and every chunk has a open shard, so - // total_occupied_blk_count is equal to the num of chunks in this pg since each chunk has a shard header. - ASSERT_EQ(hs_pg->pg_sb_->total_occupied_blk_count, pg_chunk_nums[pg_id]); - ASSERT_EQ(hs_pg->durable_entities().total_occupied_blk_count, pg_chunk_nums[pg_id]); + // all blobs deleted and gc'd; no header/footer blks since shard create is log-only + ASSERT_EQ(hs_pg->pg_sb_->total_occupied_blk_count, 0); + ASSERT_EQ(hs_pg->durable_entities().total_occupied_blk_count, 0); // after all blobs have been deleted, the pg index table should be empty ASSERT_EQ(get_valid_blob_count_in_pg(pg_id), 0); diff --git a/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp b/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp index 053d5aa5..875e50bd 100644 --- a/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp +++ b/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp @@ -95,6 +95,7 @@ using homeobject::ChunkState; using homeobject::csharedChunk; using homeobject::HeapChunkSelector; using homeobject::pg_id_t; +using homeobject::shard_id_t; using homestore::Chunk; using homestore::chunk_num_t; @@ -140,6 +141,8 @@ class HeapChunkSelectorTest : public ::testing::Test { ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_pg_id.value(), pg_id); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_v_chunk_id.value(), i); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); // test pg chunks must belong to same pdev auto pdev_id = HCS.m_chunks[p_chunk_id]->get_pdev_id(); if (last_pdev_id != 0) { @@ -181,6 +184,7 @@ TEST_F(HeapChunkSelectorTest, test_identical_layout) { const auto v_chunk_id = exVchunk->m_v_chunk_id.value(); p_chunk_id = pg_chunk_collection->m_pg_chunks[v_chunk_id]->get_chunk_id(); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j - 1); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count - j); @@ -188,27 +192,33 @@ TEST_F(HeapChunkSelectorTest, test_identical_layout) { ASSERT_TRUE(v_chunkID2.has_value()); ASSERT_EQ(v_chunk_id, v_chunkID2.value()); - // mock leader on_commit: chunk already INUSE, select_specific_chunk is a no-op - ASSERT_NE(HCS.select_specific_chunk(pg_id, v_chunk_id), nullptr); + // mock leader on_commit: chunk already INUSE with same owner, idempotent + ASSERT_NE(HCS.select_specific_chunk(pg_id, v_chunk_id, shard_id_t(j)), nullptr); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j - 1); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count - j); // mock leader rollback or on_error - ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id)); + ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, shard_id_t(j))); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count); // mock follower rollback or on_error - ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id)); + ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, shard_id_t(j))); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count); // mock follower on_commit - ASSERT_NE(HCS.select_specific_chunk(pg_id, v_chunk_id), nullptr); + ASSERT_NE(HCS.select_specific_chunk(pg_id, v_chunk_id, shard_id_t(j)), nullptr); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j - 1); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count - j); @@ -221,11 +231,12 @@ TEST_F(HeapChunkSelectorTest, test_identical_layout) { TEST_F(HeapChunkSelectorTest, test_select_specific_chunk_and_release_chunk) { for (uint16_t pg_id = 1; pg_id < 4; ++pg_id) { + const shard_id_t owner_shard_id = (shard_id_t(pg_id) << 48) | 1; // test fake - ASSERT_FALSE(HCS.release_chunk(FAKE_PG_ID, FAKE_CHUNK_ID)); - ASSERT_FALSE(HCS.release_chunk(pg_id, FAKE_CHUNK_ID)); - ASSERT_EQ(nullptr, HCS.select_specific_chunk(FAKE_PG_ID, FAKE_CHUNK_ID)); - ASSERT_EQ(nullptr, HCS.select_specific_chunk(pg_id, FAKE_CHUNK_ID)); + ASSERT_FALSE(HCS.release_chunk(FAKE_PG_ID, FAKE_CHUNK_ID, owner_shard_id)); + ASSERT_FALSE(HCS.release_chunk(pg_id, FAKE_CHUNK_ID, owner_shard_id)); + ASSERT_EQ(nullptr, HCS.select_specific_chunk(FAKE_PG_ID, FAKE_CHUNK_ID, owner_shard_id)); + ASSERT_EQ(nullptr, HCS.select_specific_chunk(pg_id, FAKE_CHUNK_ID, owner_shard_id)); auto chunk_ids = HCS.get_pg_chunks(pg_id); ASSERT_NE(chunk_ids, nullptr); @@ -233,38 +244,48 @@ TEST_F(HeapChunkSelectorTest, test_select_specific_chunk_and_release_chunk) { const chunk_num_t p_chunk_id = chunk_ids->at(v_chunk_id); auto pg_chunk_collection = HCS.m_per_pg_chunks[pg_id]; - auto p_chunkID = HCS.select_specific_chunk(pg_id, v_chunk_id); + auto p_chunkID = HCS.select_specific_chunk(pg_id, v_chunk_id, owner_shard_id); ASSERT_NE(p_chunkID, nullptr); ASSERT_EQ(p_chunkID->get_chunk_id(), p_chunk_id); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_shard_id); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2); - // test select an INUSE chunk - p_chunkID = HCS.select_specific_chunk(pg_id, v_chunk_id); + // test select an INUSE chunk with same owner (idempotent) + p_chunkID = HCS.select_specific_chunk(pg_id, v_chunk_id, owner_shard_id); ASSERT_NE(p_chunkID, nullptr); ASSERT_EQ(p_chunkID->get_chunk_id(), p_chunk_id); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_shard_id); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2); // release this chunk to HeapChunkSelector - ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id)); + ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, owner_shard_id)); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 3); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2 + 3); // test release an AVAILABLE chunk - ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id)); + ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, owner_shard_id)); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 3); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2 + 3); // select again - p_chunkID = HCS.select_specific_chunk(pg_id, v_chunk_id); + p_chunkID = HCS.select_specific_chunk(pg_id, v_chunk_id, owner_shard_id); ASSERT_NE(p_chunkID, nullptr); ASSERT_EQ(p_chunkID->get_chunk_id(), p_chunk_id); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_shard_id); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2); ASSERT_EQ(pg_id, HCS.m_chunks[p_chunk_id]->get_pdev_id()); // in this ut, pg_id is same as pdev id @@ -344,31 +365,108 @@ TEST_F(HeapChunkSelectorTest, test_recovery) { // on_shard_meta_blk_recover_completed for (uint16_t pg_id = 1; pg_id < 4; ++pg_id) { - // test recover pg heap - std::unordered_set< homestore::chunk_num_t > excluding_chunks; - excluding_chunks.emplace(0); + const shard_id_t owner_shard_id = (shard_id_t(pg_id) << 48) | 1; + // v_chunk_id=0 is open shard with owner_shard_id, v_chunk_id=1 is available + std::unordered_map< homestore::chunk_num_t, shard_id_t > inuse_chunks; + inuse_chunks.emplace(0, owner_shard_id); - ASSERT_FALSE(HCS_recovery.recover_pg_chunks_states(FAKE_PG_ID, excluding_chunks)); - ASSERT_TRUE(HCS_recovery.recover_pg_chunks_states(pg_id, excluding_chunks)); + ASSERT_FALSE(HCS_recovery.recover_pg_chunks_states(FAKE_PG_ID, inuse_chunks)); + ASSERT_TRUE(HCS_recovery.recover_pg_chunks_states(pg_id, inuse_chunks)); auto pg_it = HCS_recovery.m_per_pg_chunks.find(pg_id); ASSERT_NE(pg_it, HCS_recovery.m_per_pg_chunks.end()); auto pg_chunk_collection = pg_it->second; ASSERT_EQ(pg_chunk_collection->m_pg_chunks.size(), 2); // size won't change ASSERT_EQ(pg_chunk_collection->available_num_chunks, 1); - ASSERT_EQ(pg_chunk_collection->available_blk_count, 2); // only left v_chunk_id=1 + ASSERT_EQ(pg_chunk_collection->available_blk_count, 2); // only v_chunk_id=1 available ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::INUSE); + ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_owner_shard_id, owner_shard_id); + ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[1]->m_state, ChunkState::AVAILABLE); + ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[1]->valid()); + ASSERT_FALSE(pg_chunk_collection->m_pg_chunks[1]->m_owner_shard_id.has_value()); - const auto exVchunk = HCS_recovery.pick_most_available_blk_chunk(9999, pg_id); + // v_chunk_id=0: INUSE with correct owner → idempotent select, wrong owner → fail + ASSERT_NE(nullptr, HCS_recovery.select_specific_chunk(pg_id, 0, owner_shard_id)); + ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::INUSE); + ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); + const shard_id_t other_shard = (shard_id_t(pg_id) << 48) | 2; + ASSERT_EQ(nullptr, HCS_recovery.select_specific_chunk(pg_id, 0, other_shard)); + ASSERT_FALSE(HCS_recovery.release_chunk(pg_id, 0, other_shard)); + ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::INUSE); + ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); + ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_owner_shard_id, owner_shard_id); + + const shard_id_t recovery_owner = 9999; + const auto exVchunk = HCS_recovery.pick_most_available_blk_chunk(recovery_owner, pg_id); ASSERT_NE(exVchunk, nullptr); ASSERT_TRUE(exVchunk->m_v_chunk_id.has_value()); - auto selected_chunk = HCS_recovery.select_specific_chunk(pg_id, exVchunk->m_v_chunk_id.value()); + ASSERT_TRUE(exVchunk->valid()); + // same owner as pick_most_available_blk_chunk → idempotent + auto selected_chunk = HCS_recovery.select_specific_chunk(pg_id, exVchunk->m_v_chunk_id.value(), recovery_owner); ASSERT_NE(selected_chunk, nullptr); + ASSERT_TRUE(selected_chunk->valid()); ASSERT_EQ(selected_chunk->get_pdev_id(), pg_id); ASSERT_EQ(selected_chunk->available_blks(), 2); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[1]->m_state, ChunkState::INUSE); + ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[1]->valid()); + } +} + +TEST_F(HeapChunkSelectorTest, test_select_and_release_chunk_with_owner) { + for (uint16_t pg_id = 1; pg_id < 4; ++pg_id) { + auto chunk_ids = HCS.get_pg_chunks(pg_id); + ASSERT_NE(chunk_ids, nullptr); + const chunk_num_t v_chunk_id = 0; + const chunk_num_t p_chunk_id = chunk_ids->at(v_chunk_id); + auto pg_chunk_collection = HCS.m_per_pg_chunks[pg_id]; + + const shard_id_t owner_a = (shard_id_t(pg_id) << 48) | 1; + const shard_id_t owner_b = (shard_id_t(pg_id) << 48) | 2; + + // AVAILABLE → INUSE with owner_a + auto chunk = HCS.select_specific_chunk(pg_id, v_chunk_id, owner_a); + ASSERT_NE(chunk, nullptr); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_a); + ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); + + // same owner → idempotent, counts unchanged + chunk = HCS.select_specific_chunk(pg_id, v_chunk_id, owner_a); + ASSERT_NE(chunk, nullptr); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_a); + ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); + + // different owner → fail, state unchanged + ASSERT_EQ(nullptr, HCS.select_specific_chunk(pg_id, v_chunk_id, owner_b)); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_a); + + // wrong owner release → fail, state unchanged + ASSERT_FALSE(HCS.release_chunk(pg_id, v_chunk_id, owner_b)); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_a); + + // correct owner release → AVAILABLE, owner cleared + ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, owner_a)); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); + ASSERT_EQ(pg_chunk_collection->available_num_chunks, 3); + ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2 + 3); + + // already AVAILABLE → idempotent + ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, owner_a)); + ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); + ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); + ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); + ASSERT_EQ(pg_chunk_collection->available_num_chunks, 3); } } From 102268efdd779b5d6fe264eadce6cba7cd8a3e09 Mon Sep 17 00:00:00 2001 From: Hooper Date: Wed, 1 Jul 2026 22:34:11 +0800 Subject: [PATCH 2/3] - Instead of changing chunk state on on_shard_meta_blk_recover_completed. keep the chunk state clean(initialized) - enhance GC chunk ownership handling, distinguish between normal and recovery - CMakeList add properties to avoid running test in parallel --- src/lib/homestore_backend/CMakeLists.txt | 14 +++++++ src/lib/homestore_backend/gc_manager.cpp | 27 +++++++++++++ .../homestore_backend/heap_chunk_selector.cpp | 38 +++++-------------- .../homestore_backend/heap_chunk_selector.h | 4 +- src/lib/homestore_backend/hs_blob_manager.cpp | 13 ++++--- .../homestore_backend/hs_shard_manager.cpp | 13 +------ .../tests/test_heap_chunk_selector.cpp | 32 +++++++++------- 7 files changed, 80 insertions(+), 61 deletions(-) diff --git a/src/lib/homestore_backend/CMakeLists.txt b/src/lib/homestore_backend/CMakeLists.txt index 4361b995..4df2b8a7 100644 --- a/src/lib/homestore_backend/CMakeLists.txt +++ b/src/lib/homestore_backend/CMakeLists.txt @@ -177,3 +177,17 @@ add_test(NAME HomestoreTestGC_ShardRaceTests --override_config hs_backend_config.gc_garbage_rate_threshold=0 --override_config hs_backend_config.gc_scan_interval_sec=5 --gtest_filter=HomeObjectFixture.StalePChunkRouteAfterGC:HomeObjectFixture.StaleBlobRouteAfterSealAndGC) + +# All tests share disk, port, and shared-memory resources, so run them serially to avoid parallel conflicts. +set_tests_properties( + HomestoreTestBlob + HomestoreTestMisc + HomestoreTestReplaceMember + HomestoreTestReplaceMemberWithBaselineResync + HomestoreTestBaselineResync + HomestoreResyncTestWithFollowerRestart + HomestoreResyncTestWithLeaderRestart + FetchDataWithOriginatorGC + HomestoreTestGC + HomestoreTestGC_ShardRaceTests + PROPERTIES RUN_SERIAL TRUE) diff --git a/src/lib/homestore_backend/gc_manager.cpp b/src/lib/homestore_backend/gc_manager.cpp index f0eb9577..0a67f1c4 100644 --- a/src/lib/homestore_backend/gc_manager.cpp +++ b/src/lib/homestore_backend/gc_manager.cpp @@ -488,6 +488,33 @@ void GCManager::pdev_gc_actor::handle_recovered_gc_task( move_to_vchunk->m_pg_id = std::nullopt; move_to_vchunk->m_v_chunk_id = std::nullopt; + if (priority == static_cast< uint8_t >(task_priority::emergent)) { + // EGC recovery: log replay has not run yet so m_owner_shard_id is nullopt on all chunks. + // Find the open shard from persisted shard superblk data and set it on move_from so that + // update_vchunk_info_after_gc can copy it to move_to (INUSE requires an owner). + // The open shard may be recorded under move_from_chunk (crash before shard_superblk updated) + // or move_to_chunk (crash after shard_superblk updated). + std::optional< shard_id_t > open_shard; + for (chunk_id_t chunk_id : {move_from_chunk, move_to_chunk}) { + const auto shards = m_hs_home_object->get_shards_in_chunk(chunk_id); + if (shards.empty()) continue; + const auto last_shard_id = *(shards.rbegin()); + const auto* shard = m_hs_home_object->_get_hs_shard(last_shard_id); + if (shard && shard->info.state == ShardInfo::State::OPEN) { + open_shard = last_shard_id; + break; + } + } + if (open_shard.has_value()) { + move_from_vchunk->m_owner_shard_id = open_shard; + LOGDEBUGMOD(gcmgr, "EGC recovery: set move_from_chunk={} owner to shard=0x{:x} for replay", + move_from_chunk, open_shard.value()); + } else { + LOGWARNMOD(gcmgr, "EGC recovery: no open shard found in move_from_chunk={} or move_to_chunk={}", + move_from_chunk, move_to_chunk); + } + } + std::vector< std::pair< BlobRouteByChunkKey, BlobRouteValue > > valid_blob_indexes; if (!get_blobs_to_replace(move_to_chunk, valid_blob_indexes, 0, pg_id)) { diff --git a/src/lib/homestore_backend/heap_chunk_selector.cpp b/src/lib/homestore_backend/heap_chunk_selector.cpp index 8a0b0e6b..9741467a 100644 --- a/src/lib/homestore_backend/heap_chunk_selector.cpp +++ b/src/lib/homestore_backend/heap_chunk_selector.cpp @@ -348,22 +348,9 @@ void HeapChunkSelector::update_vchunk_info_after_gc(const chunk_num_t move_from_ // 1 change the pg_id and vchunk_id of the move_to_chunk according to metablk move_to_vchunk->m_pg_id = pg_id; move_to_vchunk->m_v_chunk_id = vchunk_id; - if (final_state == ChunkState::INUSE) { - // EGC: move_to takes over as the active chunk for the open shard and must carry its owner_shard_id. - // - // Two cases: - // Normal EGC (first run): move_to was a reserved chunk with no owner; copy owner from move_from. - // Crash-recovery replay: after crash, pg_superblk and shard_superblk already point to move_to, - // so recover_pg_chunks_states() has already set move_to.owner correctly. move_from is no - // longer listed in pg_superblk, so recover_pg_chunks_states() leaves its owner = nullopt. - // Copying from move_from would overwrite the correct owner with null — skip the copy. - if (!move_to_vchunk->m_owner_shard_id.has_value()) { - move_to_vchunk->m_owner_shard_id = move_from_vchunk->m_owner_shard_id; - } - } else { - // Normal GC: move_to becomes AVAILABLE for future shards in this PG and must have no owner. - move_to_vchunk->m_owner_shard_id = std::nullopt; - } + // GC transfers data but not ownership: move_to inherits move_from's owner_shard_id unchanged. + // (nullopt for normal GC since move_from was AVAILABLE; open shard id for EGC.) + move_to_vchunk->m_owner_shard_id = move_from_vchunk->m_owner_shard_id; // 2 update the chunk state of move_from_chunk, now it is a reserved chunk move_from_vchunk->m_pg_id = std::nullopt; @@ -375,6 +362,10 @@ void HeapChunkSelector::update_vchunk_info_after_gc(const chunk_num_t move_from_ // same as that before crash. here, the same means not new put_blob or create_shard happens to it, the data on the // chunk is the same as before. move_to_vchunk->m_state = final_state; + RELEASE_ASSERT(move_to_vchunk->valid(), + "gc task_id={}, move_to_chunk={} invalid state after gc update: state={} owner=0x{:x} pchunk={}", + task_id, move_to_chunk, move_to_vchunk->m_state, + move_to_vchunk->m_owner_shard_id.value_or(0), move_to_chunk); LOGDEBUGMOD(homeobject, "gc task_id={}, update vchunk info after gc, move_to_chunk={} now in pg={}, vchunk={}, state={}", @@ -489,8 +480,7 @@ void HeapChunkSelector::build_pdev_available_chunk_heap() { } } -bool HeapChunkSelector::recover_pg_chunks_states(pg_id_t pg_id, - const std::unordered_map< chunk_num_t, shard_id_t >& inuse_chunks) { +bool HeapChunkSelector::recover_pg_chunks_states(pg_id_t pg_id) { std::unique_lock lock_guard(m_chunk_selector_mtx); auto pg_it = m_per_pg_chunks.find(pg_id); if (pg_it == m_per_pg_chunks.end()) { @@ -505,16 +495,8 @@ bool HeapChunkSelector::recover_pg_chunks_states(pg_id_t pg_id, for (size_t v_chunk_id = 0; v_chunk_id < pg_chunks.size(); ++v_chunk_id) { auto chunk = pg_chunks[v_chunk_id]; pg_chunk_collection->m_total_blks += chunk->get_total_blks(); - auto it = inuse_chunks.find(v_chunk_id); - if (it == inuse_chunks.end()) { - chunk->m_state = ChunkState::AVAILABLE; - ++pg_chunk_collection->available_num_chunks; - pg_chunk_collection->available_blk_count += chunk->available_blks(); - - } else { - chunk->m_state = ChunkState::INUSE; - chunk->m_owner_shard_id = it->second; - } + ++pg_chunk_collection->available_num_chunks; + pg_chunk_collection->available_blk_count += chunk->available_blks(); } return true; } diff --git a/src/lib/homestore_backend/heap_chunk_selector.h b/src/lib/homestore_backend/heap_chunk_selector.h index cc032e88..cf6602f1 100644 --- a/src/lib/homestore_backend/heap_chunk_selector.h +++ b/src/lib/homestore_backend/heap_chunk_selector.h @@ -148,8 +148,8 @@ class HeapChunkSelector : public homestore::ChunkSelector { // this should be called after all pg meta blk and gc reserved chunk recovered void build_pdev_available_chunk_heap(); - // this should be called after ShardManager is initialized and get all the open shards - bool recover_pg_chunks_states(pg_id_t pg_id, const std::unordered_map< chunk_num_t, shard_id_t >& inuse_chunks); + // this should be called after ShardManager is initialized + bool recover_pg_chunks_states(pg_id_t pg_id); /** * Returns the number of available blocks of the given device id. diff --git a/src/lib/homestore_backend/hs_blob_manager.cpp b/src/lib/homestore_backend/hs_blob_manager.cpp index 2ca0b7bc..34027d21 100644 --- a/src/lib/homestore_backend/hs_blob_manager.cpp +++ b/src/lib/homestore_backend/hs_blob_manager.cpp @@ -229,10 +229,8 @@ bool HSHomeObject::local_add_blob_info(pg_id_t const pg_id, BlobInfo const& blob return false; } if (!exist_already) { - // check the shard's vchunk and pchunk id for this blob, and validate the chunk state at commit time. - // For log replay, since seal shard will trigger cp flush, so there only will be one shard's log related to one - // specific chunk. GC will not be triggered for this chunk, so we can safely check the chunk state at commit - // time. + // Validate shard vchunk/pchunk mapping for this blob and verify chunk state at commit time. + // During log replay, GC triggers a checkpoint flush, so logs after cp_lsn should not reference GC-moved blobs. homestore::chunk_num_t sb_vchunk_id; homestore::chunk_num_t sb_pchunk_id; { @@ -254,6 +252,8 @@ bool HSHomeObject::local_add_blob_info(pg_id_t const pg_id, BlobInfo const& blob blob_info.pbas.chunk_num(), sb_pchunk_id); } // check shard meta consistency + // since log replay may skip create shard log and replay the blob log directly, only check owner shard id when + // the chunk is in INUSE state const auto ext_chunk = chunk_selector()->get_pg_vchunk(pg_id, sb_vchunk_id); if (ext_chunk == nullptr) { BLOGE(tid, blob_info.shard_id, blob_info.blob_id, @@ -261,8 +261,9 @@ bool HSHomeObject::local_add_blob_info(pg_id_t const pg_id, BlobInfo const& blob blob_info.shard_id, blob_info.blob_id); RELEASE_ASSERT(false, "traceID={}, chunk not found at commit time: pg={} v_chunk={} shard=0x{:x} blob={}", tid, pg_id, sb_vchunk_id, blob_info.shard_id, blob_info.blob_id); - } else if (!ext_chunk->valid() || ext_chunk->m_state != ChunkState::INUSE || - ext_chunk->m_owner_shard_id != blob_info.shard_id || ext_chunk->get_chunk_id() != sb_pchunk_id) { + } else if (!ext_chunk->valid() || + (ext_chunk->m_state == ChunkState::INUSE && ext_chunk->m_owner_shard_id != blob_info.shard_id) || + ext_chunk->get_chunk_id() != sb_pchunk_id) { BLOGE(tid, blob_info.shard_id, blob_info.blob_id, "commit-time chunk state invalid: pg={} v_chunk={} blob={} state={} owner=0x{:x} pchunk={} " "sb_pchunk={}", diff --git a/src/lib/homestore_backend/hs_shard_manager.cpp b/src/lib/homestore_backend/hs_shard_manager.cpp index 1d3d2f56..f410d9c1 100644 --- a/src/lib/homestore_backend/hs_shard_manager.cpp +++ b/src/lib/homestore_backend/hs_shard_manager.cpp @@ -662,23 +662,14 @@ void HSHomeObject::on_shard_meta_blk_found(homestore::meta_blk* mblk, sisl::byte } void HSHomeObject::on_shard_meta_blk_recover_completed(bool success) { - std::unordered_map< homestore::chunk_num_t, shard_id_t > inuse_chunks; std::scoped_lock lock_guard(_pg_lock); for (auto& pair : _pg_map) { if (dynamic_cast< HS_PG* >(pair.second.get())->pg_state_.is_state_set(PGStateMask::DISK_DOWN)) { LOGW("pg={} is disk down, skip recover chunk state from shards", pair.first); continue; } - inuse_chunks.clear(); - inuse_chunks.reserve(pair.second->shards_.size()); - for (auto& shard : pair.second->shards_) { - if (shard->info.state == ShardInfo::State::OPEN) { - inuse_chunks.emplace(d_cast< HS_Shard* >(shard.get())->sb_->v_chunk_id, shard->info.id); - } - } - // After restart, each chunk retains at most one shard log, because every - // seal-shard commit forces a cp flush. - bool res = chunk_selector_->recover_pg_chunks_states(pair.first, inuse_chunks); + // Recover from the initialized state; chunk state and PG metrics will be updated during log replay. + bool res = chunk_selector_->recover_pg_chunks_states(pair.first); RELEASE_ASSERT(res, "Failed to recover pg chunk heap, pg={}", pair.first); } } diff --git a/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp b/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp index 875e50bd..11c8521c 100644 --- a/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp +++ b/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp @@ -363,31 +363,35 @@ TEST_F(HeapChunkSelectorTest, test_recovery) { ASSERT_EQ(chunk->get_chunk_id(), 3 + (pg_id - 1) * 3); } - // on_shard_meta_blk_recover_completed + // on_shard_meta_blk_recover_completed: all chunks set AVAILABLE; log replay updates state for (uint16_t pg_id = 1; pg_id < 4; ++pg_id) { - const shard_id_t owner_shard_id = (shard_id_t(pg_id) << 48) | 1; - // v_chunk_id=0 is open shard with owner_shard_id, v_chunk_id=1 is available - std::unordered_map< homestore::chunk_num_t, shard_id_t > inuse_chunks; - inuse_chunks.emplace(0, owner_shard_id); - - ASSERT_FALSE(HCS_recovery.recover_pg_chunks_states(FAKE_PG_ID, inuse_chunks)); - ASSERT_TRUE(HCS_recovery.recover_pg_chunks_states(pg_id, inuse_chunks)); + ASSERT_FALSE(HCS_recovery.recover_pg_chunks_states(FAKE_PG_ID)); + ASSERT_TRUE(HCS_recovery.recover_pg_chunks_states(pg_id)); auto pg_it = HCS_recovery.m_per_pg_chunks.find(pg_id); ASSERT_NE(pg_it, HCS_recovery.m_per_pg_chunks.end()); auto pg_chunk_collection = pg_it->second; - ASSERT_EQ(pg_chunk_collection->m_pg_chunks.size(), 2); // size won't change - ASSERT_EQ(pg_chunk_collection->available_num_chunks, 1); - ASSERT_EQ(pg_chunk_collection->available_blk_count, 2); // only v_chunk_id=1 available + ASSERT_EQ(pg_chunk_collection->m_pg_chunks.size(), 2); + ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); // both AVAILABLE after recovery + ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2); - ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::INUSE); - ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_owner_shard_id, owner_shard_id); + ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::AVAILABLE); ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); + ASSERT_FALSE(pg_chunk_collection->m_pg_chunks[0]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[1]->m_state, ChunkState::AVAILABLE); ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[1]->valid()); ASSERT_FALSE(pg_chunk_collection->m_pg_chunks[1]->m_owner_shard_id.has_value()); - // v_chunk_id=0: INUSE with correct owner → idempotent select, wrong owner → fail + // simulate log replay: v_chunk_id=0 claimed by open shard + const shard_id_t owner_shard_id = (shard_id_t(pg_id) << 48) | 1; + ASSERT_NE(nullptr, HCS_recovery.select_specific_chunk(pg_id, 0, owner_shard_id)); + ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::INUSE); + ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); + ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_owner_shard_id, owner_shard_id); + ASSERT_EQ(pg_chunk_collection->available_num_chunks, 1); + ASSERT_EQ(pg_chunk_collection->available_blk_count, 2); + + // idempotent select same owner, wrong owner → fail ASSERT_NE(nullptr, HCS_recovery.select_specific_chunk(pg_id, 0, owner_shard_id)); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::INUSE); ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); From ee5081f4965743898d6cddf88a14f8ada87d17d3 Mon Sep 17 00:00:00 2001 From: Hooper Date: Fri, 3 Jul 2026 17:10:42 +0800 Subject: [PATCH 3/3] fix comments: remove valid() --- src/lib/homestore_backend/gc_manager.cpp | 14 +++--- src/lib/homestore_backend/gc_manager.hpp | 1 - .../homestore_backend/heap_chunk_selector.cpp | 30 +----------- .../homestore_backend/heap_chunk_selector.h | 20 ++------ src/lib/homestore_backend/hs_blob_manager.cpp | 46 +++---------------- .../tests/test_heap_chunk_selector.cpp | 25 ---------- 6 files changed, 20 insertions(+), 116 deletions(-) diff --git a/src/lib/homestore_backend/gc_manager.cpp b/src/lib/homestore_backend/gc_manager.cpp index 0a67f1c4..b70f008c 100644 --- a/src/lib/homestore_backend/gc_manager.cpp +++ b/src/lib/homestore_backend/gc_manager.cpp @@ -505,14 +505,12 @@ void GCManager::pdev_gc_actor::handle_recovered_gc_task( break; } } - if (open_shard.has_value()) { - move_from_vchunk->m_owner_shard_id = open_shard; - LOGDEBUGMOD(gcmgr, "EGC recovery: set move_from_chunk={} owner to shard=0x{:x} for replay", - move_from_chunk, open_shard.value()); - } else { - LOGWARNMOD(gcmgr, "EGC recovery: no open shard found in move_from_chunk={} or move_to_chunk={}", - move_from_chunk, move_to_chunk); - } + RELEASE_ASSERT(open_shard.has_value(), + "EGC recovery: no open shard found in move_from_chunk={} or move_to_chunk={}", move_from_chunk, + move_to_chunk); + move_from_vchunk->m_owner_shard_id = open_shard; + LOGDEBUGMOD(gcmgr, "EGC recovery: set move_from_chunk={} owner to shard=0x{:x} for replay", move_from_chunk, + open_shard.value()); } std::vector< std::pair< BlobRouteByChunkKey, BlobRouteValue > > valid_blob_indexes; diff --git a/src/lib/homestore_backend/gc_manager.hpp b/src/lib/homestore_backend/gc_manager.hpp index 1ee48f49..abd078a6 100644 --- a/src/lib/homestore_backend/gc_manager.hpp +++ b/src/lib/homestore_backend/gc_manager.hpp @@ -291,7 +291,6 @@ class GCManager { pdev_gc_metrics& metrics() { return metrics_; } - private: private: friend class gc_task_guard; uint32_t m_pdev_id; diff --git a/src/lib/homestore_backend/heap_chunk_selector.cpp b/src/lib/homestore_backend/heap_chunk_selector.cpp index 9741467a..c74ebeeb 100644 --- a/src/lib/homestore_backend/heap_chunk_selector.cpp +++ b/src/lib/homestore_backend/heap_chunk_selector.cpp @@ -109,26 +109,17 @@ HeapChunkSelector::select_specific_chunk(const pg_id_t pg_id, const chunk_num_t return nullptr; } chunk = pg_chunks[v_chunk_id]; - if (!chunk->valid()) { - LOGERRORMOD( - homeobject, "v_chunk_id={} pg={} invalid state before select: state={} owner=0x{:x} pchunk={}", - v_chunk_id, pg_id, chunk->m_state, chunk->m_owner_shard_id.value_or(0), chunk->get_chunk_id()); - return nullptr; - } else if (chunk->m_state == ChunkState::GC) { + if (chunk->m_state == ChunkState::GC) { LOGDEBUGMOD(homeobject, "v_chunk_id={} for pg={} is pchunk_id={}, in GC state, wait and retry!", v_chunk_id, pg_id, chunk->get_chunk_id()); } else if (chunk->m_state == ChunkState::AVAILABLE) { chunk->m_state = ChunkState::INUSE; chunk->m_owner_shard_id = owner_shard_id; - RELEASE_ASSERT( - chunk->valid(), - "v_chunk_id={} pg={} invalid state after AVAILABLE→INUSE: state={} owner=0x{:x} pchunk={}", - v_chunk_id, pg_id, chunk->m_state, chunk->m_owner_shard_id.value_or(0), chunk->get_chunk_id()); --pg_chunk_collection->available_num_chunks; pg_chunk_collection->available_blk_count -= chunk->available_blks(); break; } else { - // INUSE with valid owner (guaranteed by valid()): same owner → idempotent; other owner → error. + // INUSE: same owner → idempotent; different owner → error. if (chunk->m_owner_shard_id == owner_shard_id) { break; } LOGWARNMOD(homeobject, "v_chunk_id={} for pg={} is INUSE by shard=0x{:x}, cannot select for shard=0x{:x}", @@ -163,11 +154,6 @@ bool HeapChunkSelector::release_chunk(const pg_id_t pg_id, const chunk_num_t v_c } std::scoped_lock lock(pg_chunk_collection->mtx); auto chunk = pg_chunks[v_chunk_id]; - if (!chunk->valid()) { - LOGERRORMOD(homeobject, "v_chunk_id={} pg={} invalid state before release: state={} owner=0x{:x} pchunk={}", - v_chunk_id, pg_id, chunk->m_state, chunk->m_owner_shard_id.value_or(0), chunk->get_chunk_id()); - return false; - } if (chunk->m_state == ChunkState::AVAILABLE) { return true; // idempotent for replay } @@ -179,9 +165,6 @@ bool HeapChunkSelector::release_chunk(const pg_id_t pg_id, const chunk_num_t v_c } chunk->m_state = ChunkState::AVAILABLE; chunk->m_owner_shard_id.reset(); - RELEASE_ASSERT(chunk->valid(), - "v_chunk_id={} pg={} invalid state after INUSE→AVAILABLE: state={} owner=0x{:x} pchunk={}", - v_chunk_id, pg_id, chunk->m_state, chunk->m_owner_shard_id.value_or(0), chunk->get_chunk_id()); ++pg_chunk_collection->available_num_chunks; pg_chunk_collection->available_blk_count += chunk->available_blks(); } @@ -362,10 +345,6 @@ void HeapChunkSelector::update_vchunk_info_after_gc(const chunk_num_t move_from_ // same as that before crash. here, the same means not new put_blob or create_shard happens to it, the data on the // chunk is the same as before. move_to_vchunk->m_state = final_state; - RELEASE_ASSERT(move_to_vchunk->valid(), - "gc task_id={}, move_to_chunk={} invalid state after gc update: state={} owner=0x{:x} pchunk={}", - task_id, move_to_chunk, move_to_vchunk->m_state, - move_to_vchunk->m_owner_shard_id.value_or(0), move_to_chunk); LOGDEBUGMOD(homeobject, "gc task_id={}, update vchunk info after gc, move_to_chunk={} now in pg={}, vchunk={}, state={}", @@ -542,15 +521,10 @@ HeapChunkSelector::pick_most_available_blk_chunk(shard_id_t shard_id, pg_id_t pg } auto v_chunk_id = std::distance(pg_chunks.begin(), max_it); auto& picked = pg_chunks[v_chunk_id]; - RELEASE_ASSERT(picked->valid(), "v_chunk_id={} pg={} invalid state before pick: state={} owner=0x{:x} pchunk={}", - v_chunk_id, pg_id, picked->m_state, picked->m_owner_shard_id.value_or(0), picked->get_chunk_id()); LOGDEBUGMOD(homeobject, "Picked v_chunk_id={} : [p_chunk_id={}, avail={}], shard_id=0x{:x}", v_chunk_id, picked->get_chunk_id(), picked->available_blks(), shard_id); picked->m_state = ChunkState::INUSE; picked->m_owner_shard_id = shard_id; - RELEASE_ASSERT(picked->valid(), - "v_chunk_id={} pg={} invalid state after AVAILABLE→INUSE in pick: state={} owner=0x{:x} pchunk={}", - v_chunk_id, pg_id, picked->m_state, picked->m_owner_shard_id.value_or(0), picked->get_chunk_id()); --pg_chunk_collection->available_num_chunks; pg_chunk_collection->available_blk_count -= picked->available_blks(); return picked; diff --git a/src/lib/homestore_backend/heap_chunk_selector.h b/src/lib/homestore_backend/heap_chunk_selector.h index cf6602f1..2bec584c 100644 --- a/src/lib/homestore_backend/heap_chunk_selector.h +++ b/src/lib/homestore_backend/heap_chunk_selector.h @@ -38,19 +38,7 @@ class HeapChunkSelector : public homestore::ChunkSelector { std::optional< pg_id_t > m_pg_id; std::optional< chunk_num_t > m_v_chunk_id; std::optional< shard_id_t > m_owner_shard_id; // runtime only, not persisted - bool available() const { return valid() && m_state == ChunkState::AVAILABLE; } - bool valid() const { - switch (m_state) { - case ChunkState::AVAILABLE: - return !m_owner_shard_id.has_value(); - case ChunkState::INUSE: - return m_owner_shard_id.has_value(); - case ChunkState::GC: - return true; - default: - return false; - } - } + bool available() const { return m_state == ChunkState::AVAILABLE; } }; class ExtendedVChunkComparator { @@ -86,8 +74,10 @@ class HeapChunkSelector : public homestore::ChunkSelector { csharedChunk select_chunk([[maybe_unused]] homestore::blk_count_t nblks, const homestore::blk_alloc_hints& hints); - // State machine: GC→wait+retry, AVAILABLE→INUSE(owner=shard), INUSE+same_owner→success, INUSE+other_owner→fail. - // Only leader will call this function twice during shard creation, follower will only call once. + // State machine: GC->wait+retry, AVAILABLE->INUSE(owner=shard), INUSE+same_owner->success, + // INUSE+other_owner->fail. + // During shard creation, leader touches the target chunk in two steps: + // pick_most_available_blk_chunk() then select_specific_chunk(). Follower only runs select_specific_chunk(). homestore::cshared< ExtendedVChunk > select_specific_chunk(const pg_id_t pg_id, const chunk_num_t v_chunk_id, const shard_id_t owner_shard_id); diff --git a/src/lib/homestore_backend/hs_blob_manager.cpp b/src/lib/homestore_backend/hs_blob_manager.cpp index 34027d21..e517f985 100644 --- a/src/lib/homestore_backend/hs_blob_manager.cpp +++ b/src/lib/homestore_backend/hs_blob_manager.cpp @@ -229,9 +229,8 @@ bool HSHomeObject::local_add_blob_info(pg_id_t const pg_id, BlobInfo const& blob return false; } if (!exist_already) { - // Validate shard vchunk/pchunk mapping for this blob and verify chunk state at commit time. - // During log replay, GC triggers a checkpoint flush, so logs after cp_lsn should not reference GC-moved blobs. - homestore::chunk_num_t sb_vchunk_id; + // Validate the blob was written to the pchunk the shard records. + // Catches GC-related inconsistencies (EGC moved chunk while blob was in-flight). homestore::chunk_num_t sb_pchunk_id; { std::shared_lock lock_guard(_shard_lock); @@ -239,43 +238,12 @@ bool HSHomeObject::local_add_blob_info(pg_id_t const pg_id, BlobInfo const& blob RELEASE_ASSERT(iter != _shard_map.end(), "shardID=0x{:x}, pg={}, shard=0x{:x}, shard does not exist", blob_info.shard_id, (blob_info.shard_id >> homeobject::shard_width), (blob_info.shard_id & homeobject::shard_mask)); - const auto* hs_shard = d_cast< HS_Shard* >((*iter->second).get()); - sb_vchunk_id = hs_shard->v_chunk_id(); - sb_pchunk_id = hs_shard->p_chunk_id(); - } - // check the blob's pchunk id and shard's pchunk id for this blob - if (blob_info.pbas.chunk_num() != sb_pchunk_id) { - BLOGE(tid, blob_info.shard_id, blob_info.blob_id, - "commit-time pchunk id mismatch: blob pchunk={} shard pchunk={}", blob_info.pbas.chunk_num(), - sb_pchunk_id); - RELEASE_ASSERT(false, "traceID={}, commit-time pchunk id mismatch: blob pchunk={} shard pchunk={}", tid, - blob_info.pbas.chunk_num(), sb_pchunk_id); - } - // check shard meta consistency - // since log replay may skip create shard log and replay the blob log directly, only check owner shard id when - // the chunk is in INUSE state - const auto ext_chunk = chunk_selector()->get_pg_vchunk(pg_id, sb_vchunk_id); - if (ext_chunk == nullptr) { - BLOGE(tid, blob_info.shard_id, blob_info.blob_id, - "chunk not found at commit time: pg={} v_chunk={} shard=0x{:x} blob={}", pg_id, sb_vchunk_id, - blob_info.shard_id, blob_info.blob_id); - RELEASE_ASSERT(false, "traceID={}, chunk not found at commit time: pg={} v_chunk={} shard=0x{:x} blob={}", - tid, pg_id, sb_vchunk_id, blob_info.shard_id, blob_info.blob_id); - } else if (!ext_chunk->valid() || - (ext_chunk->m_state == ChunkState::INUSE && ext_chunk->m_owner_shard_id != blob_info.shard_id) || - ext_chunk->get_chunk_id() != sb_pchunk_id) { - BLOGE(tid, blob_info.shard_id, blob_info.blob_id, - "commit-time chunk state invalid: pg={} v_chunk={} blob={} state={} owner=0x{:x} pchunk={} " - "sb_pchunk={}", - pg_id, sb_vchunk_id, blob_info.blob_id, ext_chunk->m_state, ext_chunk->m_owner_shard_id.value_or(0), - ext_chunk->get_chunk_id(), sb_pchunk_id); - RELEASE_ASSERT( - false, - "traceID={}, commit-time chunk state invalid: pg={} v_chunk={} blob={} state={} owner=0x{:x} " - "pchunk={} sb_pchunk={}", - tid, pg_id, sb_vchunk_id, blob_info.blob_id, ext_chunk->m_state, - ext_chunk->m_owner_shard_id.value_or(0), ext_chunk->get_chunk_id(), sb_pchunk_id); + sb_pchunk_id = d_cast< HS_Shard* >((*iter->second).get())->p_chunk_id(); } + RELEASE_ASSERT(blob_info.pbas.chunk_num() == sb_pchunk_id, + "traceID={}, commit-time pchunk mismatch: blob pchunk={} shard pchunk={} " + "shard=0x{:x} blob={} pg={}", + tid, blob_info.pbas.chunk_num(), sb_pchunk_id, blob_info.shard_id, blob_info.blob_id, pg_id); // The PG superblock (durable entities) will be persisted as part of HS_CLIENT Checkpoint, which is always // done ahead of the Index Checkpoint. Hence, if the index already has this entity, whatever durable diff --git a/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp b/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp index 11c8521c..0ecf60dc 100644 --- a/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp +++ b/src/lib/homestore_backend/tests/test_heap_chunk_selector.cpp @@ -141,7 +141,6 @@ class HeapChunkSelectorTest : public ::testing::Test { ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_pg_id.value(), pg_id); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_v_chunk_id.value(), i); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); // test pg chunks must belong to same pdev auto pdev_id = HCS.m_chunks[p_chunk_id]->get_pdev_id(); @@ -184,7 +183,6 @@ TEST_F(HeapChunkSelectorTest, test_identical_layout) { const auto v_chunk_id = exVchunk->m_v_chunk_id.value(); p_chunk_id = pg_chunk_collection->m_pg_chunks[v_chunk_id]->get_chunk_id(); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j - 1); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count - j); @@ -195,14 +193,12 @@ TEST_F(HeapChunkSelectorTest, test_identical_layout) { // mock leader on_commit: chunk already INUSE with same owner, idempotent ASSERT_NE(HCS.select_specific_chunk(pg_id, v_chunk_id, shard_id_t(j)), nullptr); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j - 1); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count - j); // mock leader rollback or on_error ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, shard_id_t(j))); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count); @@ -210,7 +206,6 @@ TEST_F(HeapChunkSelectorTest, test_identical_layout) { // mock follower rollback or on_error ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, shard_id_t(j))); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count); @@ -218,7 +213,6 @@ TEST_F(HeapChunkSelectorTest, test_identical_layout) { // mock follower on_commit ASSERT_NE(HCS.select_specific_chunk(pg_id, v_chunk_id, shard_id_t(j)), nullptr); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, j - 1); ASSERT_EQ(pg_chunk_collection->available_blk_count, start_available_blk_count - j); @@ -248,7 +242,6 @@ TEST_F(HeapChunkSelectorTest, test_select_specific_chunk_and_release_chunk) { ASSERT_NE(p_chunkID, nullptr); ASSERT_EQ(p_chunkID->get_chunk_id(), p_chunk_id); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_shard_id); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2); @@ -258,7 +251,6 @@ TEST_F(HeapChunkSelectorTest, test_select_specific_chunk_and_release_chunk) { ASSERT_NE(p_chunkID, nullptr); ASSERT_EQ(p_chunkID->get_chunk_id(), p_chunk_id); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_shard_id); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2); @@ -266,7 +258,6 @@ TEST_F(HeapChunkSelectorTest, test_select_specific_chunk_and_release_chunk) { // release this chunk to HeapChunkSelector ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, owner_shard_id)); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 3); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2 + 3); @@ -274,7 +265,6 @@ TEST_F(HeapChunkSelectorTest, test_select_specific_chunk_and_release_chunk) { // test release an AVAILABLE chunk ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, owner_shard_id)); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 3); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2 + 3); @@ -284,7 +274,6 @@ TEST_F(HeapChunkSelectorTest, test_select_specific_chunk_and_release_chunk) { ASSERT_NE(p_chunkID, nullptr); ASSERT_EQ(p_chunkID->get_chunk_id(), p_chunk_id); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_shard_id); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2); @@ -376,17 +365,14 @@ TEST_F(HeapChunkSelectorTest, test_recovery) { ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::AVAILABLE); - ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); ASSERT_FALSE(pg_chunk_collection->m_pg_chunks[0]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[1]->m_state, ChunkState::AVAILABLE); - ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[1]->valid()); ASSERT_FALSE(pg_chunk_collection->m_pg_chunks[1]->m_owner_shard_id.has_value()); // simulate log replay: v_chunk_id=0 claimed by open shard const shard_id_t owner_shard_id = (shard_id_t(pg_id) << 48) | 1; ASSERT_NE(nullptr, HCS_recovery.select_specific_chunk(pg_id, 0, owner_shard_id)); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::INUSE); - ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_owner_shard_id, owner_shard_id); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 1); ASSERT_EQ(pg_chunk_collection->available_blk_count, 2); @@ -394,27 +380,22 @@ TEST_F(HeapChunkSelectorTest, test_recovery) { // idempotent select same owner, wrong owner → fail ASSERT_NE(nullptr, HCS_recovery.select_specific_chunk(pg_id, 0, owner_shard_id)); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::INUSE); - ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); const shard_id_t other_shard = (shard_id_t(pg_id) << 48) | 2; ASSERT_EQ(nullptr, HCS_recovery.select_specific_chunk(pg_id, 0, other_shard)); ASSERT_FALSE(HCS_recovery.release_chunk(pg_id, 0, other_shard)); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_state, ChunkState::INUSE); - ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[0]->valid()); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[0]->m_owner_shard_id, owner_shard_id); const shard_id_t recovery_owner = 9999; const auto exVchunk = HCS_recovery.pick_most_available_blk_chunk(recovery_owner, pg_id); ASSERT_NE(exVchunk, nullptr); ASSERT_TRUE(exVchunk->m_v_chunk_id.has_value()); - ASSERT_TRUE(exVchunk->valid()); // same owner as pick_most_available_blk_chunk → idempotent auto selected_chunk = HCS_recovery.select_specific_chunk(pg_id, exVchunk->m_v_chunk_id.value(), recovery_owner); ASSERT_NE(selected_chunk, nullptr); - ASSERT_TRUE(selected_chunk->valid()); ASSERT_EQ(selected_chunk->get_pdev_id(), pg_id); ASSERT_EQ(selected_chunk->available_blks(), 2); ASSERT_EQ(pg_chunk_collection->m_pg_chunks[1]->m_state, ChunkState::INUSE); - ASSERT_TRUE(pg_chunk_collection->m_pg_chunks[1]->valid()); } } @@ -433,7 +414,6 @@ TEST_F(HeapChunkSelectorTest, test_select_and_release_chunk_with_owner) { auto chunk = HCS.select_specific_chunk(pg_id, v_chunk_id, owner_a); ASSERT_NE(chunk, nullptr); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_a); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); @@ -441,26 +421,22 @@ TEST_F(HeapChunkSelectorTest, test_select_and_release_chunk_with_owner) { chunk = HCS.select_specific_chunk(pg_id, v_chunk_id, owner_a); ASSERT_NE(chunk, nullptr); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_a); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 2); // different owner → fail, state unchanged ASSERT_EQ(nullptr, HCS.select_specific_chunk(pg_id, v_chunk_id, owner_b)); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_a); // wrong owner release → fail, state unchanged ASSERT_FALSE(HCS.release_chunk(pg_id, v_chunk_id, owner_b)); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::INUSE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_owner_shard_id, owner_a); // correct owner release → AVAILABLE, owner cleared ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, owner_a)); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 3); ASSERT_EQ(pg_chunk_collection->available_blk_count, 1 + 2 + 3); @@ -468,7 +444,6 @@ TEST_F(HeapChunkSelectorTest, test_select_and_release_chunk_with_owner) { // already AVAILABLE → idempotent ASSERT_TRUE(HCS.release_chunk(pg_id, v_chunk_id, owner_a)); ASSERT_EQ(HCS.m_chunks[p_chunk_id]->m_state, ChunkState::AVAILABLE); - ASSERT_TRUE(HCS.m_chunks[p_chunk_id]->valid()); ASSERT_FALSE(HCS.m_chunks[p_chunk_id]->m_owner_shard_id.has_value()); ASSERT_EQ(pg_chunk_collection->available_num_chunks, 3); }