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

os/bluestore: narrow cache lock range; make sure min_alloc_size p2 aligned #15911

Merged
merged 5 commits into from Jul 1, 2017

Conversation

Projects
None yet
3 participants
@xiexingguo
Member

xiexingguo commented Jun 26, 2017

No description provided.

os/bluestore: narrow cache lock range for read
It is meaningful to exclude the logger counter...

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
Show outdated Hide outdated src/os/bluestore/BlueStore.cc

@liewegas liewegas added the needs-qa label Jun 26, 2017

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 27, 2017

Member

btw, I once try to update the writing list into a map, but it turns out consuming too much memory
See below:
// we use a bare intrusive list here instead of std::map because // it uses less memory and we expect this to be very small (very // few IOs in flight to the same Blob at the same time). state_list_t writing; ///< writing buffers, sorted by seq, ascending

So obviously we shall fix the comment too, since it is misleading...

Member

xiexingguo commented Jun 27, 2017

btw, I once try to update the writing list into a map, but it turns out consuming too much memory
See below:
// we use a bare intrusive list here instead of std::map because // it uses less memory and we expect this to be very small (very // few IOs in flight to the same Blob at the same time). state_list_t writing; ///< writing buffers, sorted by seq, ascending

So obviously we shall fix the comment too, since it is misleading...

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 27, 2017

Member

E.g. please take a look at _discard() method - it might split Buffer into 2 parts and insert an additional buffer to the end of 'writing' list.

@ifed01 Do you mean BufferSpace::_discard?

I don't quite understand how it will cause disorder of writing list since the seq is generated by a TransContext.

Member

xiexingguo commented Jun 27, 2017

E.g. please take a look at _discard() method - it might split Buffer into 2 parts and insert an additional buffer to the end of 'writing' list.

@ifed01 Do you mean BufferSpace::_discard?

I don't quite understand how it will cause disorder of writing list since the seq is generated by a TransContext.

@ifed01

This comment has been minimized.

Show comment
Hide comment
@ifed01

ifed01 Jun 27, 2017

Contributor

@ifed01 Do you mean BufferSpace::_discard?

yes.

I don't quite understand how it will cause disorder of writing list since the seq is generated by a >TransContext.

Imagine you have writing list with two buffers <o1, l1, seq1> and <o2, l2, seq2> where seq2 > seq1 and you have a discard call for an extent <o, l> within {o1, o1+l1) range. This will result in truncating the first buffer and inserting the third one at the end of the writing list - AKA buffer1 split:
<o1, l1-(o-o1), seq1> <o2, l2, seq2> <o+l, l1 - ( o + l - o1), seq1 >

I.e. buffer1 split caused by _discard call breaks the list ordering. Hence there are two ways to fix: split(insert) on discard properly or handle that in write_finish.

Contributor

ifed01 commented Jun 27, 2017

@ifed01 Do you mean BufferSpace::_discard?

yes.

I don't quite understand how it will cause disorder of writing list since the seq is generated by a >TransContext.

Imagine you have writing list with two buffers <o1, l1, seq1> and <o2, l2, seq2> where seq2 > seq1 and you have a discard call for an extent <o, l> within {o1, o1+l1) range. This will result in truncating the first buffer and inserting the third one at the end of the writing list - AKA buffer1 split:
<o1, l1-(o-o1), seq1> <o2, l2, seq2> <o+l, l1 - ( o + l - o1), seq1 >

I.e. buffer1 split caused by _discard call breaks the list ordering. Hence there are two ways to fix: split(insert) on discard properly or handle that in write_finish.

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 27, 2017

Member

Imagine you have writing list with two buffers <o1, l1, seq1> and <o2, l2, seq2> where seq2 > seq1 and you have a discard call for an extent <o, l> within {o1, o1+l1) range. This will result in truncating the first buffer and inserting the third one at the end of the writing list - AKA buffer1 split:
<o1, l1-(o-o1), seq1> <o2, l2, seq2> <o+l, l1 - ( o + l - o1), seq1 >

I.e. buffer1 split caused by _discard call breaks the list ordering. Hence there are two ways to fix: split(insert) on discard properly or handle that in write_finish.

Okay, I think I got you now.
Will rebase once we get @liewegas 's feedback too.

Member

xiexingguo commented Jun 27, 2017

Imagine you have writing list with two buffers <o1, l1, seq1> and <o2, l2, seq2> where seq2 > seq1 and you have a discard call for an extent <o, l> within {o1, o1+l1) range. This will result in truncating the first buffer and inserting the third one at the end of the writing list - AKA buffer1 split:
<o1, l1-(o-o1), seq1> <o2, l2, seq2> <o+l, l1 - ( o + l - o1), seq1 >

I.e. buffer1 split caused by _discard call breaks the list ordering. Hence there are two ways to fix: split(insert) on discard properly or handle that in write_finish.

Okay, I think I got you now.
Will rebase once we get @liewegas 's feedback too.

xiexingguo added some commits Jun 26, 2017

os/bluestore: make sure min_alloc_size is power of 2 aligned
BlueStore requires that min_alloc_size must be power of 2 aligned.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
os/bluestore: narrow cache lock range for OnodeSpace::lookup
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
os/bluestore: fast exit if compression mode is set to COMP_NONE
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 27, 2017

Member

I would suggest fixing split to maintain the sorted invariant since that is extremely rare.

Member

liewegas commented Jun 27, 2017

I would suggest fixing split to maintain the sorted invariant since that is extremely rare.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 27, 2017

Member

let's fix the ordering issue and retest before merging this

Member

liewegas commented Jun 27, 2017

let's fix the ordering issue and retest before merging this

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 28, 2017

Member

let's fix the ordering issue and retest before merging this

Rebased. @liewegas Can you check if the last commit is the right cure to the ordering issue?

Member

xiexingguo commented Jun 28, 2017

let's fix the ordering issue and retest before merging this

Rebased. @liewegas Can you check if the last commit is the right cure to the ordering issue?

os/bluestore: fix writing buffers may get pinned
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

@liewegas liewegas added the needs-qa label Jun 28, 2017

@ifed01

ifed01 approved these changes Jun 29, 2017

@liewegas liewegas merged commit ef2564b into ceph:master Jul 1, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
default Build finished.
Details
make check make check succeeded
Details

@xiexingguo xiexingguo deleted the xiexingguo:wip-bluestore-20170626 branch Jul 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment