SDSTOR-22799 clean up: remove GC shard header/footer I/O, blob commit validation by Hooper9973 · Pull Request #446 · eBay/HomeObject

Hooper9973 · 2026-06-30T09:17:36Z

GC no longer writes physical shard header/footer blocks
Blob commit validates chunk state matches shard metadata

codecov-commenter · 2026-06-30T10:05:26Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 61.01695% with 23 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (dev/refactor_create_seal_shard@340788e). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/lib/homestore_backend/gc_manager.cpp	62.96%	15 Missing and 5 partials ⚠️
src/lib/homestore_backend/hs_blob_manager.cpp	50.00%	2 Missing ⚠️
src/lib/homestore_backend/heap_chunk_selector.cpp	0.00%	1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                        Coverage Diff                        @@
##             dev/refactor_create_seal_shard     #446   +/-   ##
=================================================================
  Coverage                                  ?   54.83%           
=================================================================
  Files                                     ?       36           
  Lines                                     ?     5281           
  Branches                                  ?      664           
=================================================================
  Hits                                      ?     2896           
  Misses                                    ?     2087           
  Partials                                  ?      298

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JacksonYao287 · 2026-07-01T06:24:00Z

@@ -292,9 +292,6 @@ class GCManager {
        pdev_gc_metrics& metrics() { return metrics_; }

    private:


remove this private

JacksonYao287 · 2026-07-01T06:58:06Z

        }
-        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.


no, we only flush dc_lsn, not trigger a cp flush as of now

JacksonYao287 · 2026-07-01T07:04:52Z

+        // 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);
+        }


better consolidate BLOGE and RELEASE_ASSERT into only one RELEASE_ASSERT for all the logs here

JacksonYao287 · 2026-07-02T02:36:21Z

        --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.


any issue without this change?

I met one time on my env, two case runs in parallel.

JacksonYao287 · 2026-07-02T02:44:09Z

+            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={}",


should we assert here? this is an unexpected case

Besroy · 2026-07-02T06:17:24Z

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


Not sure if the "DISK_DOWN" state will be affected

Besroy · 2026-07-02T06:22:24Z

    }
    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.


What corner case does GC refer to here? Seems a bit complex. Could we move the check to the allocation stage instead? That way, we might avoid the more complicated validation logic later.

Here checks three things:

shard pchunk, vchunk consistency

blob pba should match pchunk

chunk state check.

If moves to blob_put_get_blk_alloc_hints, here are two concerns :

we cannot check the 2 item, which has shown up in gc issue.

It's hard to define what to do if the blob is trying to alloc on a sealed shard( seal shard+put blob racing). Because a sealed shard may in GC state, which is a intermediate state. Although we can reject the request, set error to the context, it will trigger a emergency on the specific chunk. And the concern 1 cannot be resolved.

The comment means :
Not this pr involved. When I implement the PR, I found the line223:add_to_index_table , this function's behavior is:
if put a exist key, the value(pba) should be same, otherwise it will not return btree_status_t::success.
So I'm confused, what will happen, if we replay a sealed shard 's put blob log (which chunk has finished gc), the blob's currenty pbs value is different with the pbs in logs.
But GC will triggered CP flush, so this scenario won't show up.

I am not convinced of putting such complicated check on each blob commit...
it just as wired as you have a blood check at the end of each breath

only check the consistency between blob pba and shard's pchunkid

I cannot get why cannot check the 2 item, could you explain more? --- BTW checking at alloc also works for 'replay a sealed shard 's put blob log '

Here are two time spot:

get_blob_alloc_hint

on_commit

get_blob_alloc_hint will be racing with two parallel operation : 1. seal shard on commit 2.GC
Whenever you check, you can only get the temporary state.
For an example, the GC issue2: put blob to a sealed shard.

T1: alloc pba, if check at this time, all things looks good.
T2: seal shard
T3: GC, since the blob is not committed, the data will be lost.

Although this case will be fixed, we can know only the on commit is the final state, we check the consistency after inserting the blob index, if the shard chunk id match the blob's pba, the blob is good.

Besroy · 2026-07-02T06:26:35Z

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


IIUC, after this PR the shard state is either loaded when the shard SB is found, or updated by that shard’s log replay. Also, only the chunk for the open shard should be inuse (on_log_replay_done selects the specific chunk). But could blob commit hit a case where the blob’s shard has already been sealed, and the same chunk has since been taken over by a new shard, causing the shard-id check here to fail?

The major concern is to adapt log replay.

Currently all chunks are initalized after recovering meta(pg, shard)
And will select all open shard on log replay done
Here are all the logs between cp_lsn & dc_lsn:

LSN 11: create shard 1 on chunk x
LSN 12: put blob b1_1
LSN 13: seal shard1 on chunk y
LSN 14: create shard2 on chunk x
LSN 15: put blob b2_1

Current on commit logic is:

create shard, won't select a existing shard.

seal shard, seal all shard.

The recovery will like, the chunk's always be available with no owner_shard_id, before on_log_replay_done.
So I only check the owner shard when the chunk is in use.

Yes I understand this case, but will this case happen and failed check: the blob’s shard has already been sealed, and the same chunk has since been taken over by a new shard, then ext_chunk->m_owner_shard_id != blob_info.shard_id

That case will be intercepted before being inserted into the index table by comparing with sealed_lsn :
https://github.com/Hooper9973/HomeObject/blob/ee5081f4965743898d6cddf88a14f8ada87d17d3/src/lib/homestore_backend/hs_blob_manager.cpp#L306

Oh,,, then another question is : will this happen in replay senario:
T1: put blob on shard A with LSN 100
T2: seal shard A with LSN 101, shard sb: sealed_lsn=101
T3: create shard B with LSN 102, chunk owner = B
T4: crash
After crash, assume that lsn100 should replay and the blob idx not persited, then 2 case:

replay lsn100 but reject at https://github.com/Hooper9973/HomeObject/blob/ee5081f4965743898d6cddf88a14f8ada87d17d3/src/lib/homestore_backend/hs_blob_manager.cpp#L306, so blob index not udpate

if 1 pass, assert at chunk owner_shard_id check

xiaoxichen

can we target the ownership tracking as best-effort?
for example if the ownership tracking is established in-memory upon on_log_replay_done, will it simplify some of the corner cases?

The ownership tracking is something that helps us to reduce complexity and detecting issues. if we need a lot of fancy stuff to make the tracking right, even potentially more bugs and fixes in the tracking code, it doesnt make sense

xiaoxichen · 2026-07-02T16:52:48Z

    }
    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.


I am not convinced of putting such complicated check on each blob commit...
it just as wired as you have a blood check at the end of each breath

JacksonYao287 · 2026-07-06T06:51:36Z

+        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;


move_from_vchunk->m_owner_shard_id = open_shard.value()?

JacksonYao287 · 2026-07-06T06:55:33Z

 add_test(NAME HomestoreTestGC COMMAND homestore_test_gc --executor immediate --config_path ./
        --override_config hs_backend_config.enable_gc=true
        --override_config hs_backend_config.gc_enable_read_verify=true
        --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)

 # Shard-race repro tests: require chunks_per_pg=1 (forces vchunk reuse after seal) and gc_garbage_rate_threshold=0.
 # Run together since they share the same config requirements.
 add_test(NAME HomestoreTestGC_ShardRaceTests
        COMMAND homestore_test_gc --executor immediate --config_path ./
        --chunks_per_pg 1
        --override_config hs_backend_config.enable_gc=true
        --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)


suggest combine these two test into only one HomestoreTestGC

ShardRaceTests need additional parameter --chunks_per_pg 1, that's the reason I split them

JacksonYao287 · 2026-07-06T07:52:47Z

+                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;


I am not sure it is safe to return a nullptr, should we continue waiting in the loop until the chunk to be available

JacksonYao287 · 2026-07-06T07:58:58Z

        }
        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);


add an assert to check if the return value of release_chunk is true

xiaoxichen · 2026-07-06T08:42:23Z

@Hooper9973 based on the review discussion, it seems like we should first merge those cleanup codes into 0.5.6
and leave the chunk ownership tracking into 0.5.7 , what do you think?

cc @JacksonYao287 @Besroy

JacksonYao287 · 2026-07-06T08:31:49Z

    }
    if (!exist_already) {
+        // Validate the blob was written to the pchunk the shard records.
+        // Catches GC-related inconsistencies (EGC moved chunk while blob was in-flight).


egc will start only when all the appended log are committed, so I think if egc is working, there is no in-flight put_blob

JacksonYao287 · 2026-07-06T08:36:49Z

    }
    std::scoped_lock lock(pg_chunk_collection->mtx);
    auto chunk = pg_chunks[v_chunk_id];
+    if (chunk->m_state == ChunkState::AVAILABLE) {


suggest add release_assert(chunk->m_state != ChunkState::GC)

JacksonYao287 · 2026-07-06T08:51:04Z

leave the chunk ownership tracking into 0.5.7

if the committed two prs are sufficient to solve the create shard/ GC issues, I am +1 with this proposal

Besroy · 2026-07-06T09:21:51Z

@Hooper9973 based on the review discussion, it seems like we should first merge those cleanup codes into 0.5.6 and leave the chunk ownership tracking into 0.5.7 , what do you think?

cc @JacksonYao287 @Besroy

lgtm, we can merge thoese cleanup codes into 0.5.6, build a new image and test on SH as soon as possible

Hooper9973 · 2026-07-06T09:28:33Z

@Hooper9973 based on the review discussion, it seems like we should first merge those cleanup codes into 0.5.6
and leave the chunk ownership tracking into 0.5.7 , what do you think?

Sounds good, I agree with that split. I’ll prepare a new version

…ation on blob commit

Hooper9973 requested review from Besroy, JacksonYao287, koujl, xiaoxichen and yuwmao June 30, 2026 09:17

Hooper9973 force-pushed the SDSTOR-22799-opt branch 2 times, most recently from f8e31f5 to 1237535 Compare June 30, 2026 09:25

Hooper9973 force-pushed the SDSTOR-22799-opt branch 2 times, most recently from ea3c34b to 664db00 Compare July 1, 2026 00:36

JacksonYao287 reviewed Jul 1, 2026

View reviewed changes

JacksonYao287 reviewed Jul 2, 2026

View reviewed changes

Besroy reviewed Jul 2, 2026

View reviewed changes

Comment thread src/lib/homestore_backend/heap_chunk_selector.cpp Outdated

Besroy reviewed Jul 2, 2026

View reviewed changes

Comment thread src/lib/homestore_backend/heap_chunk_selector.h Outdated

Besroy reviewed Jul 2, 2026

View reviewed changes

Comment thread src/lib/homestore_backend/heap_chunk_selector.h Outdated

Besroy reviewed Jul 2, 2026

View reviewed changes

xiaoxichen reviewed Jul 2, 2026

View reviewed changes

Hooper9973 requested review from Besroy, JacksonYao287 and xiaoxichen July 6, 2026 06:45

JacksonYao287 reviewed Jul 6, 2026

View reviewed changes

Hooper9973 force-pushed the SDSTOR-22799-opt branch from ee5081f to 631ff22 Compare July 6, 2026 14:35

Remove unused shard header/footer generation during gc, and add valid…

8c74e07

…ation on blob commit

Hooper9973 force-pushed the SDSTOR-22799-opt branch from 631ff22 to 8c74e07 Compare July 6, 2026 23:59

Hooper9973 changed the title ~~SDSTOR-22799 feat: chunk ownership tracking, remove GC shard header/footer I/O, blob commit validation~~ SDSTOR-22799 clean up: remove GC shard header/footer I/O, blob commit validation Jul 7, 2026

		@@ -292,9 +292,6 @@ class GCManager {
		pdev_gc_metrics& metrics() { return metrics_; }

		private:

Uh oh!

Conversation

Hooper9973 commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaoxichen Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaoxichen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiaoxichen Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaoxichen commented Jul 6, 2026

Uh oh!

Choose a reason for hiding this comment

Hooper9973 commented Jun 30, 2026 •

edited

Loading

codecov-commenter commented Jun 30, 2026 •

edited

Loading

xiaoxichen Jul 2, 2026 •

edited

Loading

xiaoxichen Jul 2, 2026 •

edited

Loading