SDSTOR-20656: clear wbc chunk selector by nnastonen · Pull Request #912 · eBay/HomeStore

nnastonen · 2026-09-10T12:44:53Z

Create homestore with chunk selector
Create btree1 and populate it
Nodes get into wbc cache
Destroy btree1 (wbc purge is not done, also volume's chunk bitmap is not cleared)
Create btree2 and populate it
Nodes get into wbc cache and we get cache corruption, duplicate inserts

This change makes chunk release asynchronous and defers returning the chunk to the free pool until cleanup is complete.

New flow:

detach the volume's chunk list from m_volume_chunks
run asynchronous cleanup for all the chunks
in cleanup, evict all allocated blkids for the chunks from WBC
[reset the chunks' block allocator state (bitmap)]
reinsert the chunks into m_per_dev_chunks

This PR also addresses this ticket SDSTOR-25404

JacksonYao287 · 2026-09-11T03:50:22Z

    ${HOMESTORE_OBJECTS} 
 )
 target_link_libraries(homestore ${COMMON_DEPS})
+target_compile_definitions(homestore PRIVATE _PRERELEASE)


why we need this _PRERELEASE here? I think target_compile_definitions(test_index_chunk_selector PRIVATE _PRERELEASE) in tests/CmakeLists.txt is enough for the new test case

Because otherwise the code won't compile.

adding _PRERELEASE unconditionally to the homestore library itself means every build (including production Release builds) will now permanently carry _PRERELEASE: extra fault-injection checkpoints, extra assert branches, and extra debug fields get baked into the production library. That's a much bigger behavioral change than "just make the new test compile." pls correct me if I misunderstand something.

You are right. I added _PRERELEASE just to make this compile and run. Please suggest a better place/way to enable _PRERELEASE as I didn't really find how it is enabled in 7.x. I presume it is not the same as Debug build.

@szmyd since we disable prerelease in eBay/sisl#278, do you have any sugestion on this ?

JacksonYao287 · 2026-09-11T03:56:55Z

    virtual void recover(sisl::byte_view sb) = 0;
+
+    /// Remove entries for a specific chunk from wbc
+    virtual void evict_chunk_blkids(const Chunk& chunk) = 0;


NIT: if this a time-consuming task, I would suggest make it return a sisl::async::task < void >, so that we can use co_wait to wait for the completion asyncronously?

It is a time-consuming task, we call it from HomeBlocks within iomanager.run_on_forget(), check the other half of this PR: eBay/HomeBlocks#179
We don't need to wait for it as it does all the job itself.

IMO, if we do nott need wait for it and it is a time-consuming task, then it would be better to return a sisl::async::task < void >, so that we can use detach to run this time-consuming task asyncronously and not block the calling thread

iomanager.run_on_forget() is also async, it does not block the caller and I am not a big fan of detach() personally. So this is basically a stylistic matter, unless there is something I don't know about sisl::async.
Please elaborate how this would be better for this particular case.

when you submit a job by run_on_forget, it means you don`t care when the job is done and the result.

If I understand correctly, the chunk should be reused (selected by chunk selector again) only after all the chunk blk are evicted from wb_cache. so we should care when the job is done, right?

if the signature of this function is virtual sisl::async::task<void> evict_chunk_blkids(const Chunk& chunk) = 0;, then the code can be sisl::async::detach_then("evict_chunk_blkids", "return it back to chunk selector"), which means return the chunk back to selector after evict_chunk_blkids is done.

pls correct me if I misunderstand something

Yes, the lambda ran with run_on_forget() is put on a worker thread pool and runs asynchronously as soon as a worker can pick it up. There is no need to create unneeded overhead on scheduler by creating a new thread. The lambda does all the required work of releasing the chunks by itself, we do not wait for this to happen.
I still don't understand how sisl::async::task will help here.

xiaoxichen · 2026-09-14T09:01:10Z

@szmyd @shosseinimotlagh Please if you can ensure the upgrade path and compatibility are properly discussed.

NuObject is moving to V7 mostly to intake the folly and co-routine removal, which is still a persistent-compatible version (meaning , no data migration is needed upgrading from v6 to v7).

With the potential path forward of this feature, it is unlikely to be compatible with v7 as of now. I would suggest it should be in V8 with proper feature bit. Also decisions regarding how V6 can upgrade to V8 should be discussed.

szmyd · 2026-09-14T17:07:42Z

Also decisions regarding how V6 can upgrade to V8 should be discussed.

It's my understanding that we only use v3 (NuBlox 1.x/2.0) and v7 (NuObject) at the moment. We're currently working on v8 for NuBlox 2.1; so where does v6 come into play?

xiaoxichen · 2026-09-14T17:20:44Z

@szmyd sorry you are right, I mess up v7 vs v6.. Yes I am talking about v7 ->v8 compatibility

Copilot

🟡 Changes recommended

Production cleanup is not wired into chunk release, and the build and test lifecycle contain blocking defects.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Introduces chunk-level WBC eviction primitives intended to prevent stale cache entries when index chunks are reused.

Changes:

Adds allocated-block enumeration and WBC chunk eviction APIs.
Adds a deterministic chunk-reuse regression test.
Updates test helpers, build configuration, and package version.

File summaries

File	Description
`conanfile.py`	Bumps version to 8.2.1.
`src/CMakeLists.txt`	Enables prerelease compilation.
`src/include/homestore/index/wb_cache_base.hpp`	Exposes chunk eviction API.
`src/lib/device/chunk.cpp`	Enumerates allocated blocks.
`src/lib/device/chunk.h`	Declares block enumeration.
`src/lib/index/wb_cache.cpp`	Implements chunk-wide eviction.
`src/lib/index/wb_cache.hpp`	Declares eviction override.
`src/tests/CMakeLists.txt`	Registers the regression test.
`src/tests/btree_helpers/btree_test_helper.hpp`	Makes flip arguments explicit arrays.
`src/tests/test_common/homestore_test_common.hpp`	Makes flip arguments explicit arrays.
`src/tests/test_index_chunk_selector.cpp`	Adds the chunk-reuse regression test.

Review details

Files reviewed: 11/11 changed files
Comments generated: 4
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

 target_link_libraries(homestore ${COMMON_DEPS})
+target_compile_definitions(homestore PRIVATE _PRERELEASE)


JacksonYao287

thanks @nnastonen for this change, I have no more in put. @szmyd @shosseinimotlagh can you give some inputs?

JacksonYao287 · 2026-09-16T07:45:09Z

    ${HOMESTORE_OBJECTS} 
 )
 target_link_libraries(homestore ${COMMON_DEPS})
+target_compile_definitions(homestore PRIVATE _PRERELEASE)


@szmyd since we disable prerelease in eBay/sisl#278, do you have any sugestion on this ?

JacksonYao287 · 2026-09-16T07:58:20Z

    virtual void recover(sisl::byte_view sb) = 0;
+
+    /// Remove entries for a specific chunk from wbc
+    virtual void evict_chunk_blkids(const Chunk& chunk) = 0;


when you submit a job by run_on_forget, it means you don`t care when the job is done and the result.

If I understand correctly, the chunk should be reused (selected by chunk selector again) only after all the chunk blk are evicted from wb_cache. so we should care when the job is done, right?

if the signature of this function is virtual sisl::async::task<void> evict_chunk_blkids(const Chunk& chunk) = 0;, then the code can be sisl::async::detach_then("evict_chunk_blkids", "return it back to chunk selector"), which means return the chunk back to selector after evict_chunk_blkids is done.

pls correct me if I misunderstand something

codecov-commenter · 2026-09-16T16:20:41Z

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

Codecov Report

❌ Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (dev/v8.x@ed9f85d). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/lib/device/chunk.cpp	62.50%	1 Missing and 2 partials ⚠️
src/lib/index/wb_cache.cpp	75.00%	0 Missing and 2 partials ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             dev/v8.x     #912   +/-   ##
===========================================
  Coverage            ?   46.21%           
===========================================
  Files               ?      114           
  Lines               ?    12548           
  Branches            ?     5910           
===========================================
  Hits                ?     5799           
  Misses              ?     3174           
  Partials            ?     3575

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

nnastonen requested review from JacksonYao287, raakella1, shosseinimotlagh, szmyd and xiaoxichen September 10, 2026 13:07

nnastonen force-pushed the SDSTOR-20656_clear_wbc_chunk_selector_v8 branch 4 times, most recently from e2d3391 to 1d2aef4 Compare September 10, 2026 14:07

JacksonYao287 reviewed Sep 14, 2026

View reviewed changes

shosseinimotlagh requested a balanced review from Copilot September 14, 2026 18:20

Copilot started reviewing on behalf of shosseinimotlagh September 14, 2026 18:21 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

JacksonYao287 reviewed Sep 16, 2026

View reviewed changes

SDSTOR-25404: clear wbc for chunk selector

456213a

nnastonen force-pushed the SDSTOR-20656_clear_wbc_chunk_selector_v8 branch from 1d2aef4 to 456213a Compare September 16, 2026 15:42

		target_link_libraries(homestore ${COMMON_DEPS})
		target_compile_definitions(homestore PRIVATE _PRERELEASE)

Uh oh!

Conversation

nnastonen commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JacksonYao287 Sep 14, 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!

nnastonen Sep 14, 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!

JacksonYao287 Sep 14, 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!

nnastonen Sep 14, 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!

Uh oh!

xiaoxichen commented Sep 14, 2026

Uh oh!

szmyd commented Sep 14, 2026

Uh oh!

xiaoxichen commented Sep 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JacksonYao287 left a comment

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!

codecov-commenter commented Sep 16, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nnastonen commented Sep 10, 2026 •

edited

Loading

JacksonYao287 Sep 14, 2026 •

edited

Loading

nnastonen Sep 14, 2026 •

edited

Loading

JacksonYao287 Sep 14, 2026 •

edited

Loading

nnastonen Sep 14, 2026 •

edited

Loading