Skip to content

Implement homeblock write path.#86

Merged
sanebay merged 1 commit intoeBay:mainfrom
sanebay:write_path
May 15, 2025
Merged

Implement homeblock write path.#86
sanebay merged 1 commit intoeBay:mainfrom
sanebay:write_path

Conversation

@sanebay
Copy link
Contributor

@sanebay sanebay commented May 9, 2025

Write flow writes data to disk, add key value to index and flush journal entry to journal. Add test case to test the normal io flow.

@sanebay sanebay requested review from raakella1, szmyd and yamingk May 9, 2025 21:58
@codecov-commenter
Copy link

codecov-commenter commented May 9, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 86.43068% with 46 lines in your changes missing coverage. Please review.

Project coverage is 80.11%. Comparing base (0b40663) to head (ca1d0fa).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/volume_mgr.cpp 82.95% 9 Missing and 6 partials ⚠️
src/lib/volume/index.hpp 73.58% 10 Missing and 4 partials ⚠️
src/lib/listener.cpp 55.55% 4 Missing ⚠️
src/lib/volume/tests/test_volume_io.cpp 96.55% 0 Missing and 4 partials ⚠️
src/lib/index.cpp 82.35% 2 Missing and 1 partial ⚠️
src/lib/volume/tests/test_common.hpp 92.50% 1 Missing and 2 partials ⚠️
src/lib/homeblks_impl.hpp 85.71% 0 Missing and 2 partials ⚠️
src/include/homeblks/volume_mgr.hpp 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #86       +/-   ##
===========================================
+ Coverage   61.68%   80.11%   +18.43%     
===========================================
  Files          15       17        +2     
  Lines         462      835      +373     
  Branches       35       80       +45     
===========================================
+ Hits          285      669      +384     
+ Misses        158      125       -33     
- Partials       19       41       +22     

☔ View full report in Codecov by Sentry.
📢 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.

magic_num = HOMEBLKS_MSGHEADER_MAGIC;
protocol_version = HOMEBLKS_MSGHEADER_PROTOCOL_VERSION_V1;
}
HomeBlksMsgType msg_type;
Copy link
Collaborator

@yamingk yamingk May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fail to follow the need of the MessageHeader here.
It seems from the code walk through, the only field we need is the msg_type, I am also not quite sure we even need this, as READ seems doesn't require on_commit in its path, right? It returns a future and future thenValue can get the data when it is done, right? Do we need to do anything for read in on_commit path as well?

For unmap, it is async op, to free the blocks, caller doesn't need to wait for free blks to actually freed, it can be similar as read, that after index removed the lbas, unmap's future's thenValue can return the call, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for read we dont need on_commit, but unmap needs it because if we crash, we will have leaks. Unmap will write the blkid's to free and call on_commit where it do async_free_blks. But if it crashe, recovery will find unmap type and call async_free_blks again. Right MessageHeader we can avoid since we already have CRC for write payload, its only help to find any corruption in journal entries.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For unmap, we should call index->unmap directly and index will call free_blks (same as destory index). It shouldn't need to call on_commit. Unmap can write journal entry for crash recovery, on journal reply it should just call index->unmap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, unmap will do range remove of keys from index, gets the existing blkid's with filter cb, and write to journal. In on_commit we can async free the blks.

@sanebay sanebay force-pushed the write_path branch 2 times, most recently from 65c7989 to e244d81 Compare May 13, 2025 19:22
Copy link
Contributor

@raakella1 raakella1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me.

@sanebay sanebay force-pushed the write_path branch 3 times, most recently from 65cdec3 to d29b845 Compare May 14, 2025 19:44
Write flow writes data to disk, add key value to index
and flush journal entry to journal. Add test case to test
the normal io flow.
@sanebay sanebay merged commit 3297adb into eBay:main May 15, 2025
19 checks passed
@sanebay sanebay deleted the write_path branch May 15, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants