Add isblk alloced and free_blk_now api in dataservice. #826
Conversation
During replay nublocks needs to check if blks are alloced and call free_blk_now to directly free the blks from bitmap cache and not depend on cp flush.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #826 +/- ##
===========================================
+ Coverage 56.51% 66.71% +10.20%
===========================================
Files 108 110 +2
Lines 10300 13067 +2767
Branches 1402 1897 +495
===========================================
+ Hits 5821 8718 +2897
+ Misses 3894 3350 -544
- Partials 585 999 +414 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| BLKALLOC_REL_ASSERT(old_alloc_list_ptr == nullptr, "Multiple acquires concurrently?"); | ||
| return (m_disk_bm->serialize(m_align_size)); | ||
| return (m_disk_bm->serialize(m_align_size, true)); |
There was a problem hiding this comment.
If we don't reuse the serialized buffer here, it will also impact normal IO load, right?
Can we only set this to true for recovery path, in which it will only impact the log replay?
There was a problem hiding this comment.
It doesnt normal IO path. Its only during CP flush. I tried to do using is_intialized, but it still crashes with CRC error. so keeping copy for now.
bool force_copy = hs()->is_initializing() ? true : false;
return (m_disk_bm->serialize(m_align_size, force_copy));
| return f; | ||
| } | ||
|
|
||
| std::error_code BlkDataService::free_blk_now(MultiBlkId const& bids) { |
There was a problem hiding this comment.
Using a new API is okay, I am thinking do we want to re-use async_free_blk and pass additional parameter is_recovery which is defaulted to false, and set to true during log_replay, and in async_free_blk, if is_recovery is true, we call m_vdev->free_blk(bids, nullptr /cp_ctx/); and this should fix the issue, right?
in vdev::free_blk, if ctx is nullptr, it will go ahead to varsize blkallocator and just free the blk from both cache and bitmap, same as this new API is doing.
There was a problem hiding this comment.
Ideally the logic of is_recovery is internal to homestore, so we shouldnt expose it outside.
There was a problem hiding this comment.
not necessary the case the we shouldn't expose recovery to HomeBlks. We are exposing recovery logic to HomeObject and HomeBlks already, in recovery path, we do different code path and we have recovery callback for HomeBlocks and HomeObjects to recovery PGs, volumes, etc.
| return std::make_error_code(std::errc::resource_unavailable_try_again); | ||
| } else { | ||
| auto cpg = hs()->cp_mgr().cp_guard(); | ||
| m_vdev->free_blk(bids, s_cast< VDevCPContext* >(cpg.context(cp_consumer_t::BLK_DATA_SVC)), true /* free_now */); |
There was a problem hiding this comment.
If we want a new API instead of re-use async_free_blk, which is also fine, could we set cp_ctx to nullptr without taking the cp_guard, it should do the work as it is for m_vdev->free_blk, right?
There was a problem hiding this comment.
cp_guard was kept for safety, so that it wont add to current CP, if its has already started. During recovery, CP's will be running.
3 participants
During replay nublocks needs to check if blks are alloced and call free_blk_now to directly free the blks from bitmap cache and not depend on cp flush.