-
Notifications
You must be signed in to change notification settings - Fork 17
refactor_create/seal_shard #340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
78f3f1a to
a6d7c1f
Compare
a6d7c1f to
8330325
Compare
| // +1 here means the created shard should have the capacity to hold at least one blob, otherwise we refuse this | ||
| // request. | ||
| const auto v_chunk_id = v_chunkID.value(); | ||
| const static uint64_t shard_super_blk_count{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we simply use get_reserved_blks() ?
If we want to protect the case that reserved_bytes_in_chunk set to zero, make this logical into get_reserved_blks() or simply force it to reserve a few MB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to our current logic, only seal_shard can use the reserved space(default 4 blks), which can make sure the seal_shard can be done in any case.
if we allow create shard to use the reserved space, it will break the above consumption?
sorry, I am not quite clear about you suggestion, or let me ask this in another way, how to guarantee the blk can be successfully allocated if we use get_reserved_blks()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean using the get_reserved_blks() which default to 16MB , if any chunk has less than get_reserved_blks() bytes we simply dont use it. Do not need to calculate the size of Shard Header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here, I use const static on purpose to avoid it to be calculated every time. it will be calculated only once.
| homestore::BlkAllocStatus alloc_status; | ||
| auto gc_mgr = gc_manager(); | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possbile we failed to get enough space in first emergent GC(which already cleared all written-but-not-referenced data)? Seems like we dont need a while(true).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theoretically, there are some very corner cases that we failed to get enough space in first emergent GC.
for example, if we have two stale push_data req and each of them will try to allocate 4 blks, and meanwhile, in the target chunk, there only left exact 4-blk free space, other blks are all valid.
1 the first stale push_data take 4 blks in the target chunk, which lead to the no_space_left when we try to allocate blk for shard header, and we trigger emergent gc to free 4 blks
2 the second stale push_data comes after emergent gc and take the 4 blks just freed, then when trying to allocate blk for shard header, the second no_space_left occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering Leader will not create shard on the chunk if chunk avaialbe space is lower than given threshold, is this still possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering Leader will not create shard on the chunk if chunk avaialbe space is lower than given threshold
the available space of a chunk when committing create_shard might be different from that when being selected for creating_shard because of the staled push_data request(put_blob).
Actually, for both leader and follower, this corner case might happen. this root cause is that we don`t exactly know how many staled push_data request are in filght , and when they will try to allocate blk.
a while-true loop here will not bring too much cost, since almost all the alloc_blk will succeed in the first time.
cfce547 to
4fd93bd
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
==========================================
- Coverage 63.15% 58.21% -4.94%
==========================================
Files 32 35 +3
Lines 1900 4301 +2401
Branches 204 519 +315
==========================================
+ Hits 1200 2504 +1304
- Misses 600 1533 +933
- Partials 100 264 +164 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
09fe0b9 to
7ed3511
Compare
|
@xiaoxichen I have address your comments in the latest commit, ptal |
c7e7dfd to
3247d3c
Compare
xiaoxichen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Lets bake it in SH before merging. The thinking is we might have more tiny fixes to be merge recently , want to avoid taking this out to production.
sure, I will let you know after I have run storage hammer for several rounds and confident to merge it |
7338428 to
a0a5afa
Compare
fe5ac37 to
f3a369e
Compare
f3a369e to
c1d3e02
Compare
1 change create/seal shard to log only , so no push_data happens for these two requests.
2 add a check for on_commit put_blob to avoid putting blob to a sealed shard.
3 move select_specific_chunk(mark chunk to inuse state) to on_commit of create_shard.
4 write shard footer for seal shard in baseline resync
5 add a sealed_lsn for shard meta blk, which is not compatible with previous version