SDSTOR-22799 clean up: remove GC shard header/footer I/O, blob commit validation

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"

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)

src/lib/homestore_backend/gc_manager.hpp

@@ -291,10 +291,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;

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) {

src/lib/homestore_backend/hs_blob_manager.cpp

@@ -229,6 +229,20 @@ bool HSHomeObject::local_add_blob_info(pg_id_t const pg_id, BlobInfo const& blob
         return false;
     }
     if (!exist_already) {
+        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));
+            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
         // counters updated as part of the update would have been persisted already in PG superblock. So if we were

src/lib/homestore_backend/tests/hs_gc_tests.cpp

@@ -74,15 +74,13 @@ TEST_F(HomeObjectFixture, BasicGC) {
             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) {
-                // TODO: GC will not persist shard header/footer futher,
-                // temporarily comment blk count check.
-                // total_blob_occupied_blk_count += 2; /*header and footer*/
+                // shard create/seal is log-only; no header/footer blocks written
                 for (const auto& [_, blk_count] : shard_blob_ids_map[shard_id]) {
                     total_blob_occupied_blk_count += blk_count;
                 }
             }
             // check pg durable entities
-            // ASSERT_EQ(hs_pg->durable_entities().total_occupied_blk_count, total_blob_occupied_blk_count);
+            ASSERT_EQ(hs_pg->durable_entities().total_occupied_blk_count, total_blob_occupied_blk_count);
 
             // check pg index table, the valid blob index count should be equal to the blob count
             ASSERT_EQ(get_valid_blob_count_in_pg(pg_id), pg_blob_id[pg_id]);
@@ -117,7 +115,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;
@@ -192,14 +190,13 @@ TEST_F(HomeObjectFixture, BasicGC) {
         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;
             }
         }
 
-        // 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);
+        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);
     }
 
     restart();
@@ -241,13 +238,12 @@ TEST_F(HomeObjectFixture, BasicGC) {
         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;
             }
         }
-        // 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);
+        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);
     }
 
     // delete remaining blobs
@@ -627,13 +623,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 +678,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 +718,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);