handle missing pdev when reading #405
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xiaoxichen
reviewed
May 7, 2024
xiaoxichen
previously approved these changes
May 7, 2024
src/lib/device/virtual_dev.cpp
Outdated
| return uint64_cast(b.blk_num()) * block_size() + uint64_cast((*chunk)->start_offset()); | ||
| } | ||
|
|
||
| bool VirtualDev::is_chunk_loaded(cshared< Chunk >& chunk) const { |
There was a problem hiding this comment.
only chunk id is needed?
There was a problem hiding this comment.
i wanted to set the parameter to only chunk_id. but if that, we need to get the chunk_id before
if (sisl_unlikely(!is_chunk_available(chunk)))
so , puttingthe action of getting chunk id inside is_chunk_available itself seems more clean.
since it generates no impact to the logic, we can keep it like this and change it if necessary in the future.
src/lib/device/device.h
Outdated
| } | ||
|
|
||
| Chunk* get_chunk_mutable(uint32_t chunk_id) { | ||
| std::unique_lock lg{m_vdev_mutex}; | ||
| return (chunk_id == INVALID_CHUNK_ID) ? nullptr : m_chunks[chunk_id].get(); | ||
| // if a pdev is misssing when restart, chunk_id from client might be larger than m_chunks.size() | ||
| if (chunk_id == INVALID_CHUNK_ID || chunk_id >= m_chunks.size()) return nullptr; |
There was a problem hiding this comment.
maybe m_chunks.index_exists(chunk_id)?
src/lib/device/virtual_dev.hpp
Outdated
| @@ -299,6 +299,7 @@ class VirtualDev { | |||
|
|
|||
| private: | |||
| uint64_t to_dev_offset(BlkId const& b, Chunk** chunk) const; | |||
| bool is_chunk_loaded(cshared< Chunk >& chunk) const; | |||
There was a problem hiding this comment.
maybe is_chunk_exists or is_chunk_available is a better naming? loaded sounds like if not you can load again:)
JacksonYao287
force-pushed
the
fix-missing-pdev
branch
from
df70eec to
a7de024
Compare
JacksonYao287
force-pushed
the
fix-missing-pdev
branch
from
a7de024 to
e9c0b4c
Compare
|
@xiaoxichen I address your comment, PTAL |
xiaoxichen
approved these changes
May 8, 2024
|
pls add some commit message when merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#400
in some scenarios, homestore might be restarted with missing pdev(for example, breakfix), thus all the chunks in that pdev will not be loaded. so we need to deal with this case.
1 for read from client, we need to check whether the chunk in the provided blkid is loaded and proceed accordingly.
2 for write , we will almost not encounter this case since we will never allocate blk for a missing pdev. but for the sake of completeness, check is also added .