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: miscellaneous fixes to BitAllocator #12696

Merged
merged 2 commits into from Jan 18, 2017

Conversation

Projects
None yet
3 participants
@xiexingguo
Member

xiexingguo commented Dec 28, 2016

@chhabaramesh Some more improvements to the BitAllocator submodule.
These patches have already passed my local tests. Please take a look when you are available.

Thanks.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Dec 28, 2016

Member

retest this please

Member

liewegas commented Dec 28, 2016

retest this please

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Dec 31, 2016

Member
[ RUN      ] BitAllocator.test_bmap_alloc_concurrent
Spawning 16 threads for parallel test. Mode Cont = 0.....
Starting thread 0Starting thread 1Starting thread 2Starting thread 3Starting thread 4Starting thread 5Starting thread 6Starting thread 7Starting thread 8Starting thread 9Starting thread 10Starting thread 11Starting thread 12Starting thread 13Starting thread 14Starting thread 15/home/sage/src/ceph/src/os/bluestore/BitAllocator.cc: In function 'void BitAllocator::free_blocks_dis(int64_t, ExtentList*)' thread 7f856969b700 time 2016-12-31 11:14:08.064051
/home/sage/src/ceph/src/os/bluestore/BitAllocator.cc: 1484: FAILED assert(get_used_blocks() >= 0)
 ceph version 11.1.0-6413-g78707bd (78707bd3a400b064aa9b148cdbdc7f8e50b371cf)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x79) [0x557c6cf705b9]
 2: (()+0x13bf74) [0x557c6cf59f74]
 3: (do_work_dis(BitAllocator*)+0x137) [0x557c6cf2af77]
 4: (worker(void*)+0x34) [0x557c6cf2b1f4]
 5: (()+0x76ca) [0x7f856f0806ca]
 6: (clone()+0x5f) [0x7f856bb5af6f]
 NOTE: a copy of the executable, or `objdump -rdS ` is needed to interpret this.
2016-12-31 11:14:08.064672 7f856969b700 -1 /home/sage/src/ceph/src/os/bluestore/BitAllocator.cc: In function 'void BitAllocator::free_blocks_dis(int64_t, ExtentList*)' thread 7f856969b700 time 2016-12-31 11:14:08.064051
Member

liewegas commented Dec 31, 2016

[ RUN      ] BitAllocator.test_bmap_alloc_concurrent
Spawning 16 threads for parallel test. Mode Cont = 0.....
Starting thread 0Starting thread 1Starting thread 2Starting thread 3Starting thread 4Starting thread 5Starting thread 6Starting thread 7Starting thread 8Starting thread 9Starting thread 10Starting thread 11Starting thread 12Starting thread 13Starting thread 14Starting thread 15/home/sage/src/ceph/src/os/bluestore/BitAllocator.cc: In function 'void BitAllocator::free_blocks_dis(int64_t, ExtentList*)' thread 7f856969b700 time 2016-12-31 11:14:08.064051
/home/sage/src/ceph/src/os/bluestore/BitAllocator.cc: 1484: FAILED assert(get_used_blocks() >= 0)
 ceph version 11.1.0-6413-g78707bd (78707bd3a400b064aa9b148cdbdc7f8e50b371cf)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x79) [0x557c6cf705b9]
 2: (()+0x13bf74) [0x557c6cf59f74]
 3: (do_work_dis(BitAllocator*)+0x137) [0x557c6cf2af77]
 4: (worker(void*)+0x34) [0x557c6cf2b1f4]
 5: (()+0x76ca) [0x7f856f0806ca]
 6: (clone()+0x5f) [0x7f856bb5af6f]
 NOTE: a copy of the executable, or `objdump -rdS ` is needed to interpret this.
2016-12-31 11:14:08.064672 7f856969b700 -1 /home/sage/src/ceph/src/os/bluestore/BitAllocator.cc: In function 'void BitAllocator::free_blocks_dis(int64_t, ExtentList*)' thread 7f856969b700 time 2016-12-31 11:14:08.064051
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Dec 31, 2016

Member

hmm nevermind, got it without this too

Member

liewegas commented Dec 31, 2016

hmm nevermind, got it without this too

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jan 9, 2017

Member

@varadakari care to review?

Member

liewegas commented Jan 9, 2017

@varadakari care to review?

@chhabaramesh

This comment has been minimized.

Show comment
Hide comment
@chhabaramesh

chhabaramesh Jan 10, 2017

Contributor

@xiexingguo , @liewegas , I reviewed the changes. Some of the changes are not required that I commented on. @xiexingguo please drop those changes and update the pull request.

Contributor

chhabaramesh commented Jan 10, 2017

@xiexingguo , @liewegas , I reviewed the changes. Some of the changes are not required that I commented on. @xiexingguo please drop those changes and update the pull request.

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jan 10, 2017

Member

@chhabaramesh You reviewed and commented on #12661 instead of this one...

Member

xiexingguo commented Jan 10, 2017

@chhabaramesh You reviewed and commented on #12661 instead of this one...

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo
Member

xiexingguo commented Jan 12, 2017

@chhabaramesh

This comment has been minimized.

Show comment
Hide comment
@chhabaramesh

chhabaramesh Jan 13, 2017

Contributor

@xiexingguo , I have given my comments on individual commits in this pull requests.

summary:

os/bluestore/BitAllocator: kill find_any_free_bits()
and

os/bluestore: replace m_max_alloc_size with m_max_blocks

looks ok and other two changes are not required.

Contributor

chhabaramesh commented Jan 13, 2017

@xiexingguo , I have given my comments on individual commits in this pull requests.

summary:

os/bluestore/BitAllocator: kill find_any_free_bits()
and

os/bluestore: replace m_max_alloc_size with m_max_blocks

looks ok and other two changes are not required.

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jan 13, 2017

Member

@xiexingguo , I have given my comments on individual commits in this pull requests.

Well, I can not see anything about that. Github bug, perhaps?
Dropped the other two changes anyway.

Member

xiexingguo commented Jan 13, 2017

@xiexingguo , I have given my comments on individual commits in this pull requests.

Well, I can not see anything about that. Github bug, perhaps?
Dropped the other two changes anyway.

@xiexingguo xiexingguo added the needs-qa label Jan 13, 2017

xiexingguo added some commits Dec 28, 2016

os/bluestore: replace m_max_alloc_size with m_max_blocks
So we can get rid of the annoying transfer each time we
try to add an extent into an ExtentList.
Also simplify the add_extents() method a little.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
os/bluestore/BitAllocator: kill find_any_free_bits()
Which has no consumers.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@chhabaramesh

This comment has been minimized.

Show comment
Hide comment
@chhabaramesh

chhabaramesh Jan 13, 2017

Contributor

@xiexingguo , possible and surprising :(. I have comments 3 days ago.
" chhabaramesh commented 3 days ago

@xiexingguo , @liewegas , I reviewed the changes. Some of the changes are not required that I commented on. @xiexingguo please drop those changes and update the pull request."

But my current comment has typo and apologies for confusion.

The changes related to

  1. os/bluestore/BitAllocator: kill find_any_free_bits() and
  2. remove m_lock from release function are required.

The changes related to:

  1.  os/bluestore: replace m_max_alloc_size with m_max_blocks  and
    
  2. and change related to bmap index while iterating through the bmaps is not required.
Contributor

chhabaramesh commented Jan 13, 2017

@xiexingguo , possible and surprising :(. I have comments 3 days ago.
" chhabaramesh commented 3 days ago

@xiexingguo , @liewegas , I reviewed the changes. Some of the changes are not required that I commented on. @xiexingguo please drop those changes and update the pull request."

But my current comment has typo and apologies for confusion.

The changes related to

  1. os/bluestore/BitAllocator: kill find_any_free_bits() and
  2. remove m_lock from release function are required.

The changes related to:

  1.  os/bluestore: replace m_max_alloc_size with m_max_blocks  and
    
  2. and change related to bmap index while iterating through the bmaps is not required.

@liewegas liewegas merged commit 2125221 into ceph:master Jan 18, 2017

3 checks passed

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

@xiexingguo xiexingguo deleted the xiexingguo:xxg-wip-improve-loops branch Jan 19, 2017

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