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.

Sign up for GitHub

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

Jump to bottom

os/bluestore: fix memory leak during bit_alloc testing #9935

Merged
merged 3 commits into from Jun 30, 2016
Merged

os/bluestore: fix memory leak during bit_alloc testing #9935

merged 3 commits into from Jun 30, 2016

Conversation

xiexingguo
Copy link
Member

@xiexingguo xiexingguo commented Jun 26, 2016 •
edited

Perviously as zone_size_block and span_size are identical, so we could use
span_size only to simplify the calcuation.

Now that the zone_size_block and span_size are both configurable, so
it is necessary to treat them respectively.

@xiexingguo xiexingguo force-pushed the xxg-wip-make-zone-configurable branch 3 times, most recently from 8a8260f to d9b5fcc Compare June 26, 2016 05:56
@xiexingguo
Copy link
Member Author

@chhabaramesh Can you look? Thanks!

@xiexingguo xiexingguo force-pushed the xxg-wip-make-zone-configurable branch from d9b5fcc to eee408b Compare June 26, 2016 07:56
@chhabaramesh
Copy link
Contributor

@xiexingguo , I will review today.

chhabaramesh
chhabaramesh reviewed Jun 27, 2016
View reviewed changes
src/common/config_opts.h
@@ -971,6 +971,7 @@ OPTION(bluestore_kvbackend, OPT_STR, "rocksdb")
OPTION(bluestore_allocator, OPT_STR, "bitmap") // stupid | bitmap
OPTION(bluestore_freelist_type, OPT_STR, "bitmap") // extent | bitmap
OPTION(bluestore_freelist_blocks_per_key, OPT_INT, 128)
OPTION(bluestore_bitmapallocator_blocks_per_zone, OPT_INT, 1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment on configurable valid values.

Rest in this commit looks good.

@xiexingguo xiexingguo force-pushed the xxg-wip-make-zone-configurable branch from eee408b to f36fb04 Compare June 28, 2016 11:41
@xiexingguo
Copy link
Member Author

@chhabaramesh I have done upgrading this patch as the way you wish, and it works now.

I am sorry that the commits are ordered and there is no easy way to rebase. So it may take you some time to review all of them, again.

Also I run through the UT manually in my local testbed, since Jenkins is unhappy agian, see below:

[root@gitbuilder-ceph-rpm-centos7-amd64-basic /home/bitmaptest/ceph-dev/src]# ./unittest_bit_alloc                      
2016-06-28 19:41:03.257055 7fe3c7e9cd00 -1 did not load config file, using default settings.
[==========] Running 5 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 5 tests from BitAllocator
[ RUN      ] BitAllocator.test_bmap_iter
[       OK ] BitAllocator.test_bmap_iter (0 ms)
[ RUN      ] BitAllocator.test_bmap_entry
[       OK ] BitAllocator.test_bmap_entry (1 ms)
[ RUN      ] BitAllocator.test_zone_alloc
[       OK ] BitAllocator.test_zone_alloc (3 ms)
[ RUN      ] BitAllocator.test_bmap_alloc
[       OK ] BitAllocator.test_bmap_alloc (58705 ms)
[ RUN      ] BitAllocator.test_bmap_alloc_concurrent
Spawning 16 threads for parallel test.....
Starting thread 0Starting thread 1Allocating in tid 0.
Starting thread 2Starting thread 3Allocating in tid 1.
Allocating in tid 2.
Starting thread 4Allocating in tid 3.
Starting thread 5Allocating in tid 4.
Starting thread 6Allocating in tid 5.
Allocating in tid 6.
Starting thread 7Starting thread 8Starting thread 9Allocating in tid 8.
Allocating in tid 7.
Starting thread 10Allocating in tid 9.
Starting thread 11Allocating in tid 10.
Starting thread 12Allocating in tid 11.
Starting thread 13Allocating in tid 12.
Starting thread 14Allocating in tid 13.
Starting thread 15Allocating in tid 14.
Allocating in tid 15.
Freeing in tid 0 65536 blocks.
Freeing in tid 9 65536 blocks.
Freeing in tid 13 65536 blocks.
Freeing in tid 3 65536 blocks.
Freeing in tid 10 65536 blocks.
Freeing in tid 14 65536 blocks.
Freeing in tid 6 65536 blocks.
Freeing in tid 11 65536 blocks.
Freeing in tid 1 65536 blocks.
Freeing in tid 7 65536 blocks.
Freeing in tid 2 65536 blocks.
Freeing in tid 4 65536 blocks.
Freeing in tid 8 65536 blocks.
Freeing in tid 15 65536 blocks.
Freeing in tid 12 65536 blocks.
Freeing in tid 5 65536 blocks.
Done tid 0 iter 2.
Allocating in tid 0.
Done tid 13 iter 2.
Allocating in tid 13.
Done tid 9 iter 2.
Allocating in tid 9.
Done tid 11 iter 2.
Allocating in tid 11.
Done tid 14 iter 2.
Allocating in tid 14.
Done tid 10 iter 2.
Allocating in tid 10.
Done tid 3 iter 2.
Allocating in tid 3.
Done tid 6 iter 2.
Allocating in tid 6.
Done tid 1 iter 2.
Allocating in tid 1.
Done tid 4 iter 2.
Allocating in tid 4.
Done tid 7 iter 2.
Allocating in tid 7.
Done tid 8 iter 2.
Allocating in tid 8.
Done tid 12 iter 2.
Allocating in tid 12.
Done tid 2 iter 2.
Allocating in tid 2.
Done tid 15 iter 2.
Allocating in tid 15.
Done tid 5 iter 2.
Allocating in tid 5.
Freeing in tid 0 65536 blocks.
Done tid 0 iter 1.
Allocating in tid 0.
Freeing in tid 13 65536 blocks.
Freeing in tid 9 65536 blocks.
Freeing in tid 14 65536 blocks.
Freeing in tid 3 65536 blocks.
Freeing in tid 6 65536 blocks.
Freeing in tid 10 65536 blocks.
Freeing in tid 11 65536 blocks.
Freeing in tid 4 65536 blocks.
Freeing in tid 7 65536 blocks.
Freeing in tid 8 65536 blocks.
Freeing in tid 1 65536 blocks.
Freeing in tid 2 65536 blocks.
Freeing in tid 15 65536 blocks.
Freeing in tid 12 65536 blocks.
Freeing in tid 5 65536 blocks.
Done tid 13 iter 1.
Allocating in tid 13.
Done tid 9 iter 1.
Allocating in tid 9.
Done tid 11 iter 1.
Allocating in tid 11.
Done tid 14 iter 1.
Allocating in tid 14.
Done tid 3 iter 1.
Allocating in tid 3.
Done tid 10 iter 1.
Allocating in tid 10.
Done tid 6 iter 1.
Allocating in tid 6.
Done tid 7 iter 1.
Allocating in tid 7.
Done tid 1 iter 1.
Allocating in tid 1.
Done tid 8 iter 1.
Allocating in tid 8.
Done tid 15 iter 1.
Allocating in tid 15.
Done tid 2 iter 1.
Allocating in tid 2.
Done tid 5 iter 1.
Allocating in tid 5.
Done tid 12 iter 1.
Allocating in tid 12.
Done tid 4 iter 1.
Allocating in tid 4.
Freeing in tid 0 65536 blocks.
Done tid 0 iter 0.
Freeing in tid 13 65536 blocks.
Done tid 13 iter 0.
Freeing in tid 9 65536 blocks.
Done tid 9 iter 0.
Freeing in tid 14 65536 blocks.
Freeing in tid 3 65536 blocks.
Freeing in tid 11 65536 blocks.
Freeing in tid 6 65536 blocks.
Freeing in tid 10 65536 blocks.
Freeing in tid 7 65536 blocks.
Freeing in tid 8 65536 blocks.
Freeing in tid 2 65536 blocks.
Freeing in tid 1 65536 blocks.
Freeing in tid 15 65536 blocks.
Freeing in tid 5 65536 blocks.
Freeing in tid 4 65536 blocks.
Freeing in tid 12 65536 blocks.
Done tid 11 iter 0.
Done tid 6 iter 0.
Done tid 3 iter 0.
Done tid 14 iter 0.
Done tid 2 iter 0.
Done tid 1 iter 0.
Done tid 10 iter 0.
Done tid 15 iter 0.
Done tid 5 iter 0.
Done tid 12 iter 0.
Done tid 7 iter 0.
Done tid 8 iter 0.
Done tid 4 iter 0.
[       OK ] BitAllocator.test_bmap_alloc_concurrent (20149 ms)
[----------] 5 tests from BitAllocator (78858 ms total)

[----------] Global test environment tear-down
[==========] 5 tests from 1 test case ran. (78858 ms total)
[  PASSED  ] 5 tests.
[root@gitbuilder-ceph-rpm-centos7-amd64-basic /home/bitmaptest/ceph-dev/src]# 
[root@gitbuilder-ceph-rpm-centos7-amd64-basic /home/bitmaptest/ceph-dev/src]# 
[root@gitbuilder-ceph-rpm-centos7-amd64-basic /home/bitmaptest/ceph-dev/src]# 
[root@gitbuilder-ceph-rpm-centos7-amd64-basic /home/bitmaptest/ceph-dev/src]# 
[root@gitbuilder-ceph-rpm-centos7-amd64-basic /home/bitmaptest/ceph-dev/src]# make unittest_bluefs
  CXX      test/objectstore/unittest_bluefs-test_bluefs.o
./make_version -g ./.git_version
if [ -n "$NO_VERSION" ] ; then \
        ./make_version -g ./.git_version -c ./ceph_ver.h -n ; \
else \
        ./make_version -g ./.git_version -c ./ceph_ver.h ; \
fi
  CXXLD    unittest_bluefs
[root@gitbuilder-ceph-rpm-centos7-amd64-basic /home/bitmaptest/ceph-dev/src]# ./unittest_bluefs 
2016-06-28 19:49:13.019931 7ff5f7900ec0 -1 did not load config file, using default settings.
2016-06-28 19:49:13.021345 7ff5f7900ec0 -1 WARNING: the following dangerous and experimental features are enabled: *
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from BlueFS
[ RUN      ] BlueFS.mkfs
[       OK ] BlueFS.mkfs (283 ms)
[ RUN      ] BlueFS.mkfs_mount
[       OK ] BlueFS.mkfs_mount (256 ms)
[ RUN      ] BlueFS.write_read
[       OK ] BlueFS.write_read (256 ms)
[ RUN      ] BlueFS.small_appends
2016-06-28 19:49:13.867522 7ff5f7900ec0 -1 bdev(ceph_test_bluefs.tmp.block.280452.4) aio_write pwritev error: (22) Invalid argument
[       OK ] BlueFS.small_appends (757 ms)
[----------] 4 tests from BlueFS (1552 ms total)

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

@xiexingguo
Copy link
Member Author

xiexingguo commented Jun 28, 2016 •
edited

@chhabaramesh
Since the test cases are much more complicated, I decrease the iteration of test round to half and
it now drops to a acceptable level, 78858 ms in my local test bed.

@xiexingguo xiexingguo changed the title os/bluestore: make zone size of bit allocator configurable os/bluestore: make hierarchies of bit allocator tree configurable Jun 28, 2016
chhabaramesh
chhabaramesh reviewed Jun 28, 2016
View reviewed changes
src/os/bluestore/BitMapAllocator.cc
@@ -23,8 +23,7 @@ BitMapAllocator::BitMapAllocator(int64_t device_size, int64_t block_size)
: m_num_uncommitted(0),
m_num_committing(0)
{
int64_t zone_size_blks = 1024; // Change it later

int64_t zone_size_blks = g_conf->bluestore_bitmapallocator_blocks_per_zone;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good

@chhabaramesh
Copy link
Contributor

@xiexingguo , I am sorry but it is really difficult to review so many commits with one or two line changes.
Rest is still ok, but span size and zone size change is important and we should to it as single commit and separate from rest of changes.

  1. Other changes if separated from span size related changes, are almost ready to checkin.
  2. It will be clean and more meaningful to do span size related changes in single commit and different pull request. That includes configurable zones, size, span size, all changes to make that work.

@xiexingguo
Copy link
Member Author

@chhabaramesh Okay, I'll try to re-organize these changes, please hold on.

@xiexingguo
os/bluestore: fix memory leak during bit alloc ut
06c48e4 
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo
os/bluestore: make block array of test_bmap_alloc self-adapting
1af00b1 
So we are be able change total_blocks or zone_size flexibly and
thus append more test cases.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo
os/bluestore: promote output to derr if out of memory
f35b05b 
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo xiexingguo force-pushed the xxg-wip-make-zone-configurable branch 3 times, most recently from 7c4e258 to bf79a9e Compare June 29, 2016 04:52
@xiexingguo
Copy link
Member Author

@chhabaramesh See again? It is pretty clear now, I guess.

@chhabaramesh
Copy link
Contributor

@xiexingguo , please separate the configurable zone size and span size changes and changes related to that in separate single commit. Rest small fixes can be single commit. You can squash the changes in to single commit

@xiexingguo
Copy link
Member Author

@chhabaramesh What do you mean? I have done just as you suggested.
Are you still looking at an obsolete version?

@xiexingguo
Copy link
Member Author

@chhabaramesh There are only 5 commits left, mind you.

@chhabaramesh
Copy link
Contributor

chhabaramesh commented Jun 29, 2016 •
edited

@xiexingguo apologies, I looked at older version, will review by EOD.

@xiexingguo
Copy link
Member Author

@chhabaramesh ... seems you apology to the wrong person, but never mind:-)

@chhabaramesh
Copy link
Contributor

:)

chhabaramesh
chhabaramesh reviewed Jun 29, 2016
View reviewed changes
src/os/bluestore/BitAllocator.cc
@@ -707,7 +707,7 @@ void BitMapAreaIN::init(int64_t total_blocks, int64_t area_idx, bool def)
BitMapArea **children = new BitMapArea*[num_child];
int i = 0;
for (i = 0; i < num_child - 1; i++) {
if (m_level <= 2) {
if (m_level < 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Level 0 is zone, level 1 is leaf. So when this node is <= 2 ( non leaf and not zone) we create child as leaf. Looks correct to me

@xiexingguo
Copy link
Member Author

retest this please

@chhabaramesh
Copy link
Contributor

@xiexingguo , please create separate pull request for first three changes and those can be merged.

  1. "os/bluestore: fix wrongly initialized instance of BitMapArea" : this one is not needed I think.
  2. Create separate pull request for last change related to configurable zone size. I need more time to review this one.

@liewegas first three commits are ok to go:

@xiexingguo     os/bluestore: fix memory leak during bit alloc ut
        06c48e4
@xiexingguo     os/bluestore: make block array of test_bmap_alloc self-adapting
        1af00b1
@xiexingguo     os/bluestore: promote output to derr if out of memory
        f35b05b

@xiexingguo
Copy link
Member Author

@chhabaramesh
Oh, sorry, I got you.

@xiexingguo
Copy link
Member Author

Will drop that one now, thanks! @chhabaramesh

@xiexingguo xiexingguo force-pushed the xxg-wip-make-zone-configurable branch from bf79a9e to f35b05b Compare June 30, 2016 06:38
@xiexingguo xiexingguo changed the title os/bluestore: make hierarchies of bit allocator tree configurable os/bluestore: fix memory leak during bit_alloc testing Jun 30, 2016
@xiexingguo
Copy link
Member Author

@liewegas This one is safe to merge now.

@liewegas liewegas merged commit 0ab6480 into ceph:master Jun 30, 2016
@xiexingguo xiexingguo deleted the xxg-wip-make-zone-configurable branch June 30, 2016 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bluestore bug-fix
Projects
None yet
Milestone
No milestone
3 participants
@xiexingguo @chhabaramesh @liewegas