SDSTOR-11579. expose chunk and chunkSelector Header #157
Conversation
JacksonYao287
force-pushed
the
migrate-chunk-to-include-folder
branch
from
d7dc214 to
1a0db6d
Compare
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #157 +/- ##
==========================================
- Coverage 54.10% 53.96% -0.14%
==========================================
Files 96 98 +2
Lines 8922 8941 +19
Branches 1112 1116 +4
==========================================
- Hits 4827 4825 -2
- Misses 3685 3706 +21
Partials 410 410
☔ View full report in Codecov by Sentry. |
|
|
||
| namespace homestore { | ||
| RoundRobinChunkSelector::RoundRobinChunkSelector(bool dynamic_chunk_add) : m_dynamic_chunk_add{dynamic_chunk_add} { | ||
| RELEASE_ASSERT_EQ(dynamic_chunk_add, false, | ||
| "Dynamically adding chunk to chunkselector is not supported, need RCU to make it thread safe"); | ||
| } | ||
|
|
||
| void RoundRobinChunkSelector::add_chunk(cshared< Chunk >& chunk) { m_chunks.push_back(chunk); } | ||
| void RoundRobinChunkSelector::add_chunk(VChunk chunk) { m_chunks.push_back(chunk); } |
There was a problem hiding this comment.
Can you do std::move(chunk) instead of copy VChunk, which does increment and decrement of atomic.
There was a problem hiding this comment.
it is back to the https://ebay-eng.slack.com/archives/CAVQXEAKS/p1694203171225199 discussion, vchunk only has single member <cshared_ptr>
| void foreach_chunks(std::function< void(cshared< Chunk >&) >&& cb) override; | ||
| void add_chunk(VChunk chunk) override; | ||
| Chunk* select_chunk(blk_count_t nblks, const blk_alloc_hints& hints) override; | ||
| void foreach_chunks(std::function< void(VChunk&) >&& cb) override; |
There was a problem hiding this comment.
Is there a way to avoid std::function, to auto& and make use of lambda. If not, we don't want to call foreach_chunks on very critical path. I know it is a micro optimization, but one that accumulates over time.
There was a problem hiding this comment.
it is declared in a virtual class as a param of a virtual function. it tried , but the compiler throws an error that we can not use auto& or auto&& as parameter in a virtual function declaration.
we can keep it like this for now , and find another method to change it to lambda until it is necessary in the critical path
| ChunkSelector() = default; | ||
| virtual void add_chunk(VChunk chunk) = 0; | ||
| virtual void foreach_chunks(std::function< void(VChunk&) >&& cb) = 0; | ||
| virtual Chunk* select_chunk(blk_count_t nblks, const blk_alloc_hints& hints) = 0; |
There was a problem hiding this comment.
Unrelated to derive or not-derive, which you can decide, is there any challenge or reason that we need to expose two different types VChunk/Chunk* to consumer with add/select?
Can we either return VChunk or Chunk for both add/select apis?
There was a problem hiding this comment.
customer code can use
ChunkSelector::add_chunk(chunk *)
chunk *p_chunk = ChunkSelector:select()
There was a problem hiding this comment.
@yamingk i change it to return cshared< Chunk > in both add/select apis to keep the style consistent, and do the transition inside homestore
JacksonYao287
force-pushed
the
migrate-chunk-to-include-folder
branch
from
849de5f to
87070a1
Compare
| @@ -197,7 +197,7 @@ BlkAllocStatus VirtualDev::do_alloc_blk(blk_count_t nblks, const blk_alloc_hints | |||
| size_t attempt{0}; | |||
|
|
|||
| do { | |||
| chunk = m_chunk_selector->select(nblks, hints); | |||
| chunk = m_chunk_selector->select_chunk(nblks, hints).get(); | |||
There was a problem hiding this comment.
@hkadayam This is dangerous as raw ptr/shared ptr are mixed used.
The cshare was born in PhysicalDev::create_chunks and all the way pass to chunkSelector, however the consumer part(allocator, etc) use raw ptr get from the cshare.
It is functioning fine as a reference to every chunk was kept at m_chunks_map but it is a bad practice. Maybe lets have a ticket to move those *chunk to cshare ?
There was a problem hiding this comment.
I don't agree with this change of doing a .get() and select_chunk() returning a shared_ptr. Why incrementing atomic reference when all you are doing is accessing an in-memory member. I am not asking to change now, but will change in next PR.
I would rather return a chunk_id and look it up on the all chunks in vdev. If vdev doesn't maintain all chunks, then I would rather ask it to return a reference to chunks. This is a critical path for homestore, don't want unneccessary atomic reference increment and decrements.
There was a problem hiding this comment.
the upstream (PhysicalDev::create_chunks ) use shared_ptr, if we dont want ref counting we can change the interface of add/select to ref/raw_ptr. It could work as pdev holds the shared_ptr that ensuring the chunks will not be de-construct, just not safe.
No description provided.