Skip to content
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:Fix size calculation in bitallocator #10377

Merged
merged 1 commit into from Jul 21, 2016

Conversation

chhabaramesh
Copy link
Contributor

Internal size and external size are different got Bitallocator.
Hence creating separate function.

Signed-off-by: Ramesh Chander Ramesh.Chander@sandisk.com

@chhabaramesh
Copy link
Contributor Author

Passed all three tests:

  1. unit test bit_alloc
  2. unit test alloc
  3. ceph_test_objectstore:

[==========] Running 8 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 8 tests from Allocator/AllocTest

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

[==========] Running 6 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 5 tests from BitAllocator

.....
[ OK ] BitAllocator.test_bmap_alloc_concurrent (9344 ms)
[----------] 5 tests from BitAllocator (71344 ms total)

[----------] 1 test from BitAllocator2
[ RUN ] BitAllocator2.test_bmap_alloc
[ OK ] BitAllocator2.test_bmap_alloc (0 ms)
[----------] 1 test from BitAllocator2 (0 ms total)

[----------] Global test environment tear-down
[==========] 6 tests from 2 test cases ran. (71344 ms total)
[ PASSED ] 6 tests.

2016-07-20 23:25:35.969472 7ffff7fd4680 -1 WARNING: the following dangerous and experimental features are enabled: *
[==========] Running 236 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 236 tests from ObjectStore/StoreTest
[ RUN ] ObjectStore/StoreTest.collect_metadata/0

......
==> rm -r store_test_temp_dir
[ OK ] ObjectStore/StoreTest.TooManyBlobsTest/3 (100 ms)
[----------] 236 tests from ObjectStore/StoreTest (1315409 ms total)

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

@chhabaramesh
Copy link
Contributor Author

@markhpc , this is fix for unit test case failure for allocator.

@xiexingguo
Copy link
Member

looks good to me:-)

@xiexingguo
Copy link
Member

One more question:

this adjustment is required to return correct blocks in stats.

How does this fix also address this?

@@ -529,7 +529,7 @@ class BitAllocator:public BitMapAreaIN{
void free_blocks_dis(int64_t num_blocks, ExtentList *block_list);
bool is_allocated_dis(ExtentList *blocks, int64_t num_blocks);

int64_t size() {
int64_t total_blocks() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be a const method.

@chhabaramesh
Copy link
Contributor Author

now since we have separate function for get_free in BitMapAllocator, caller of get_free will not get m_extra_blocks. Rest internal function will consider m_extra_blocks.

allocator::get_free is used by bluestore stats.

@xiexingguo
Copy link
Member

Got it. Thanks for the explanation.

std::vector<AllocExtent> extents = std::vector<AllocExtent>
(alloc->size(), AllocExtent(-1, -1));
int64_t blk_size = 1024;
std::vector<AllocExtent> extents = std::vector<AllocExtent>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use auto here.

Signed-off-by: Ramesh Chander <Ramesh.Chander@sandisk.com>
@chhabaramesh
Copy link
Contributor Author

@tchaikov , addressed all comments and tested again.

@tchaikov tchaikov merged commit 0555aba into ceph:master Jul 21, 2016
@tchaikov tchaikov self-assigned this Jul 21, 2016
@dmick
Copy link
Member

dmick commented Jul 21, 2016

This definitely should have been two commits. @chhabaramesh, please try to limit commits to one issue and make them smaller. @tchaikov, please try to request that in review.

@chhabaramesh
Copy link
Contributor Author

@dmick sure, will do suggested for future pull requests.

@tchaikov
Copy link
Contributor

@dmick sure, will do. thanks for the reminder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants