os/bluestore/BlueStore.cc: merge overlapping/adjacent regions before read #24820
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
ifed01
reviewed
Oct 30, 2018
src/test/objectstore/store_test.cc
Outdated
| @@ -7359,13 +7359,14 @@ int main(int argc, char **argv) { | |||
| g_ceph_context->_conf.set_val_or_die("filestore_op_thread_suicide_timeout", "10000"); | |||
| //g_ceph_context->_conf.set_val_or_die("filestore_fiemap", "true"); | |||
| g_ceph_context->_conf.set_val_or_die("bluestore_fsck_on_mkfs", "false"); | |||
| g_ceph_context->_conf.set_val_or_die("bluestore_fsck_on_mount", "false"); | |||
| g_ceph_context->_conf.set_val_or_die("bluestore_fsck_on_umount", "false"); | |||
| g_ceph_context->_conf.set_val_or_die("bluestore_fsck_on_mount", "true"); | |||
There was a problem hiding this comment.
Are these config changes intentional? I think you should rather create a test case with these settings to reproduce the issue. But I suggest to refrain from changing the settings globally.
src/os/bluestore/BlueStore.cc
Outdated
| uint64_t r_off = 0; | ||
| uint64_t r_len = 0; | ||
| bufferlist bl; | ||
| std::list<region_t> regs; // original reqeusts |
src/os/bluestore/BlueStore.cc
Outdated
| typedef list<region_t> regions2read_t; | ||
| typedef map<BlueStore::BlobRef, regions2read_t> blobs2read_t; | ||
| typedef list<read_req_t> merged_regions2read_t; | ||
| typedef map<BlueStore::BlobRef, merged_regions2read_t> blob2mread_t; |
There was a problem hiding this comment.
you may want to extend blobs2read_t instead of introducing one more map, e.g.
typedef map<BlueStore::BlobRef, pair<regions2read_t, merged_regions2read_t>>
Hence save on the second map operations(allocs, lookup etc)
|
@ifed01 done |
ifed01
approved these changes
Nov 1, 2018
|
this change could be related to the test failures. dropping it from my test batch. |
…read Fixes: http://tracker.ceph.com/issues/36625 Signed-off-by: Yang Honggang <yanghonggang@umcloud.com>
|
After I refined the merge logical, all StoreTest cases are passed now. |
liewegas
added a commit
that referenced
this pull request
Nov 8, 2018
* refs/pull/24820/head: os/bluestore/BlueStore.cc: merge overlapping/adjacent regions before read Reviewed-by: Igor Fedotov <ifedotov@suse.com>
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.
Fixes: http://tracker.ceph.com/issues/36625
Signed-off-by: Yang Honggang yanghonggang@umcloud.com