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: cleanup BitAllocator #12661

Merged
merged 6 commits into from Jan 15, 2017

Conversation

Projects
None yet
4 participants
@xiexingguo
Member

xiexingguo commented Dec 26, 2016

No description provided.

xiexingguo added some commits Dec 24, 2016

os/bluestore/BitAllocator: miscellaneous cleanups
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
os/bluestore/BitAllocator: end scope of std::hex properly
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Dec 26, 2016

Member

@chhabaramesh Can you look?

Member

xiexingguo commented Dec 26, 2016

@chhabaramesh Can you look?

@chhabaramesh

This comment has been minimized.

Show comment
Hide comment
@chhabaramesh

chhabaramesh Dec 26, 2016

Contributor

@xiexingguo , the changes look good to me. thanks for taking this up.

Contributor

chhabaramesh commented Dec 26, 2016

@xiexingguo , the changes look good to me. thanks for taking this up.

@chhabaramesh

This comment has been minimized.

Show comment
Hide comment
@chhabaramesh

chhabaramesh Dec 26, 2016

Contributor

@varadakari @xiexingguo , this function is just to hide the mode of Allocator and unconditionally do the required locking when in serial or concurrent mode.

Contributor

chhabaramesh commented Dec 26, 2016

@varadakari @xiexingguo , this function is just to hide the mode of Allocator and unconditionally do the required locking when in serial or concurrent mode.

@chhabaramesh

This comment has been minimized.

Show comment
Hide comment
@chhabaramesh

chhabaramesh Dec 26, 2016

Contributor

@xiexingguo please make sure: unittest_bit_alloc, unittest_alloc, ceph_test_objectstore passes with your changes :-).

Contributor

chhabaramesh commented Dec 26, 2016

@xiexingguo please make sure: unittest_bit_alloc, unittest_alloc, ceph_test_objectstore passes with your changes :-).

@varadakari

This comment has been minimized.

Show comment
Hide comment
@varadakari

varadakari Dec 26, 2016

Contributor

@chhabaramesh But that is shutdown method, i don't follow you on hiding the mode of allocator, that shouldn't matter in shutdown time right? or i am not following the shutdown correctly here.

Contributor

varadakari commented Dec 26, 2016

@chhabaramesh But that is shutdown method, i don't follow you on hiding the mode of allocator, that shouldn't matter in shutdown time right? or i am not following the shutdown correctly here.

@chhabaramesh

This comment has been minimized.

Show comment
Hide comment
@chhabaramesh

chhabaramesh Dec 26, 2016

Contributor

@varadakari , I did not notice it was in shutdown call. The reason of taking lock here and unlock is to make sure we wait for any on going calls before doing shutdown.

Contributor

chhabaramesh commented Dec 26, 2016

@varadakari , I did not notice it was in shutdown call. The reason of taking lock here and unlock is to make sure we wait for any on going calls before doing shutdown.

Show outdated Hide outdated src/os/bluestore/BitAllocator.cc
os/bluestore/BitAllocator: fix potential null pointer access
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Dec 28, 2016

Member

Passed all local tests:

[----------] 67 tests from ObjectStore/StoreTest (4829982 ms total)

[----------] Global test environment tear-down
[==========] 67 tests from 1 test case ran. (4829983 ms total)
[  PASSED  ] 67 tests.
Member

xiexingguo commented Dec 28, 2016

Passed all local tests:

[----------] 67 tests from ObjectStore/StoreTest (4829982 ms total)

[----------] Global test environment tear-down
[==========] 67 tests from 1 test case ran. (4829983 ms total)
[  PASSED  ] 67 tests.
@varadakari

This comment has been minimized.

Show comment
Hide comment
@varadakari

varadakari Dec 29, 2016

Contributor

@xiexingguo i think we should clean up the shutdown method. if that is only locking and unlocking just of the sake of making sure one was holding the lock(i think allocators were shutdown when all the threads are completed), we should assert or wait for all the threads to complete in a meaningful way.

Contributor

varadakari commented Dec 29, 2016

@xiexingguo i think we should clean up the shutdown method. if that is only locking and unlocking just of the sake of making sure one was holding the lock(i think allocators were shutdown when all the threads are completed), we should assert or wait for all the threads to complete in a meaningful way.

xiexingguo added some commits Dec 26, 2016

os/bluestore/BitAllocator: kill decr_idx() method
Which has no consumers.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
os/bluestore/BitAllocator: drop unused parameter from child_check_n_l…
…ock()

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
os/bluestore/BitAllocator: shutdown BitAllocator in a more graceful way
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Dec 29, 2016

Member

we should assert or wait for all the threads to complete in a meaningful way.

Done.

Member

xiexingguo commented Dec 29, 2016

we should assert or wait for all the threads to complete in a meaningful way.

Done.

@liewegas liewegas requested a review from varadakari Jan 11, 2017

@liewegas liewegas removed the request for review from varadakari Jan 13, 2017

@liewegas liewegas merged commit 278b1e2 into ceph:master Jan 15, 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-cleanup-bitalloc branch Jan 16, 2017

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