Skip to content

os/bluestore: new bitmap allocator #21825

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
Merged
merged 16 commits into from
Jun 12, 2018
Merged

os/bluestore: new bitmap allocator #21825

merged 16 commits into from
Jun 12, 2018
+2,509 −3,002

Conversation

ifed01
Copy link
Contributor

@ifed01 ifed01 commented May 4, 2018 •
edited
Loading

Some slides with both design overview and some performance numbers are at
https://docs.google.com/presentation/d/1_1Otkgv76fbCU2Zogjz748sEAG-1Nfiidbb6mgTON-A/edit?usp=sharing

@markhpc
Copy link
Member

markhpc commented May 10, 2018

@ifed01 Once you've got them (after the performance meeting?) can we include test results here?

@ifed01
Copy link
Contributor Author

ifed01 commented May 10, 2018

@markhpc - sure.

@ifed01
Copy link
Contributor Author

ifed01 commented May 10, 2018

@markhpc - I've just inserted a link for my slides to this PR's descripiton.

@ifed01 ifed01 force-pushed the wip-ifed-bitmap-alloc branch from 0abbda6 to 28e85ed Compare May 10, 2018 16:03
@ifed01
Copy link
Contributor Author

ifed01 commented May 10, 2018

retest this please

liewegas
liewegas reviewed May 11, 2018
View reviewed changes
src/os/bluestore/Allocator.cc
@@ -14,7 +15,7 @@ Allocator *Allocator::create(CephContext* cct, string type,
if (type == "stupid") {
return new StupidAllocator(cct);
} else if (type == "bitmap") {
return new BitMapAllocator(cct, size, block_size);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should just remove the old implementation since it's not even accessible. That also lets you rename the class BitmapAllocator

liewegas
liewegas reviewed May 11, 2018
View reviewed changes
src/os/bluestore/fastbmap_allocator_impl.cc
@@ -0,0 +1,397 @@
#include "fastbmap_allocator_impl.h"
Copy link
Member

Choose a reason for hiding this comment

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

pelase include the modelines at the top of the file.. it avoids some of the whitespace inconsistencies below (tab vs 8 spaces)

liewegas
liewegas reviewed May 11, 2018
View reviewed changes
src/os/bluestore/fastbmap_allocator_impl.h Outdated
{
uint64_t prev_allocated = *allocated;
uint64_t d = CHILD_PER_SLOT;
assert((hint % d) == 0);
Copy link
Member

Choose a reason for hiding this comment

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

couldn't the caller pass in any hint they want?

@ifed01 ifed01 force-pushed the wip-ifed-bitmap-alloc branch 2 times, most recently from df61a43 to 50b2b92 Compare May 14, 2018 13:13
@ifed01
Copy link
Contributor Author

ifed01 commented May 14, 2018

@liewegas - updated

@liewegas
Copy link
Member

This is excellent work! Let's see how it does in QA.

@ifed01 ifed01 force-pushed the wip-ifed-bitmap-alloc branch from 50b2b92 to f7417df Compare May 17, 2018 18:44
@ifed01
Copy link
Contributor Author

ifed01 commented May 18, 2018

@liewegas - should we temporary switch default allocator to bitmap to run QA suite?

@liewegas
Copy link
Member

Yeah. In fact we probably want to switch the default regardless if the new code is stable.

@ifed01 ifed01 force-pushed the wip-ifed-bitmap-alloc branch from d4ff963 to 2c89149 Compare May 22, 2018 12:06
@liewegas liewegas changed the title WIP: os/bluestore: new bitmap allocator os/bluestore: new bitmap allocator May 23, 2018
@liewegas
Copy link
Member

retest this please

@liewegas
Copy link
Member 

   -12> 2018-05-26 20:48:44.039 7f769fc05a00  1 bluefs mount
   -11> 2018-05-26 20:48:44.039 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 BitmapFastAllocator 0x2625a0000/100000
   -10> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x4b4000~100000
    -9> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x5f8000~100000
    -8> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x800000~c00000
    -7> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x1570000~200000
    -6> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x1800000~200000
    -5> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x1e64000~100000
    -4> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x2e6c000~100000
    -3> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x315c000~300000
    -2> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x35a8000~100000
    -1> 2018-05-26 20:48:44.040 7f769fc05a00 10 fbmap_alloc 0x0x55c1eac00870 init_add_free 0x3800000~400000
     0> 2018-05-26 20:48:44.048 7f769fc05a00 -1 *** Caught signal (Aborted) **
 in thread 7f769fc05a00 thread_name:ceph_test_objec

 ceph version 14.0.0-26-gb4db059 (b4db05979ac71616866d30500bf3ba26cd0992ce) nautilus (dev)
 1: (()+0x585f50) [0x55c1e406df50]
 2: (()+0xf6d0) [0x7f7696bae6d0]
 3: (gsignal()+0x37) [0x7f7692bc5277]
 4: (abort()+0x148) [0x7f7692bc6968]
 5: (()+0x492f93) [0x55c1e3f7af93]
 6: (BlueFS::_replay(bool, bool)+0xf3d) [0x55c1e4003dad]
 7: (BlueFS::mount()+0x1d9) [0x55c1e40075b9]
 8: (BlueStore::_open_db(bool, bool)+0x15ae) [0x55c1e3f36b1e]
 9: (BlueStore::_fsck(bool, bool)+0xab1) [0x55c1e3f596a1]

/a/sage-2018-05-26_16:13:44-rados-wip-sage2-testing-2018-05-26-0833-distro-basic-smithi/2594412

Note that I also saw another ceph-test-objectstore failure today, http://tracker.ceph.com/issues/24319 ... first one in a long time. This crash looks related to this PR, though!

@ifed01
Copy link
Contributor Author

ifed01 commented May 28, 2018

@liewegas - could you please share full ceph log or at least assert message text?

@liewegas
Copy link
Member

No assertion, just the above. @ifed01 you can look at the full log(s) (everything we've got) on the teuthology host at the /a/... path above... ping me on irc if you don't already have access.

@ifed01 ifed01 force-pushed the wip-ifed-bitmap-alloc branch from 2c89149 to 710006d Compare June 1, 2018 19:01
@ifed01
Copy link
Contributor Author

ifed01 commented Jun 1, 2018

@liewegas - resulting extent alignment when min_length > min_alloc_size has been fixed. Would you rerun QA please.

@liewegas
Copy link
Member

liewegas commented Jun 4, 2018

http://pulpito.ceph.com/sage-2018-06-01_21:35:10-rados-wip-sage3-testing-2018-06-01-1421-distro-basic-smithi/
looks good!

@liewegas
Copy link
Member

liewegas commented Jun 5, 2018

@ifed01 any concerns before we merge this?

@liewegas liewegas requested a review from varadakari June 5, 2018 13:54
@aclamk aclamk self-requested a review June 5, 2018 15:01
@liewegas
Copy link
Member

http://pulpito.ceph.com/sage-2018-06-11_02:34:30-rados-wip-sage3-testing-2018-06-10-1344-distro-basic-smithi/

this has gone through a half dozen runs in the last week and looks solid.

@varadakari
Copy link
Contributor

In general, it looks good to me. Need to do some fragmentations runs and see the cpu time spent.

varadakari
varadakari approved these changes Jun 11, 2018
View reviewed changes
Copy link
Contributor

@varadakari varadakari left a comment

Choose a reason for hiding this comment

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

LGTM

src/os/bluestore/BitmapFastAllocator.cc Outdated
#undef dout_prefix
#define dout_prefix *_dout << "fbmap_alloc 0x" << this << " "

BitmapFastAllocator::BitmapFastAllocator(CephContext* _cct,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename to BitmapAllocator?

ifed01 added 16 commits June 12, 2018 14:48
@ifed01
test/fio: dump mempool on job completion
16a9a0d 
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
test/allocator: get rid off bitmap allocator specifics and introduce new
dcd3061 
performance test cases

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: add new bitmap allocator
631041a 
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
test/allocator: move bluestore allocator's benchmarks to a standalone UT
ac2ec80 
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: fix improper access to a BitmapFastAllocator::last_pos
0c69672 
from multiple threads.

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: align BitMap allocator's init_rm_free/init_add_free par…
1b6a56e 
…ameters with min_alloc_size

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: remove original bitmap allocator
95bd72f 
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: default to bitmap allocator for bluestore/bluefs
231b7dd 
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: more verbose logging in new bitmap allocator
160c730 
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: cosmetic cleanup in new bitmap allocator.
3db9d9a 
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: properly respect min_length when analysing partially free
288bbd4 
slots in bitmap allocator.

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: cosmetic new allocator internal method rename.
fa06ff2 
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: respect min_length as allocation granularity for new
8a188b2 
bitmap allocator.
It was used a real minimum threshold before this fix which allowed e.g.
allocated extent length to be equal to min_length + 1.

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
test/objectstore/unitetest_fastbmap_allocator: replace ASSERT_TRUE with
69ccb43 
ASSERT_EQ

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: perform allocations aligned with min_length in new bitm…
51974c4 
…ap allocator

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01
os/bluestore: rename new bitmap allocator class to BitmapAllocator.
a8e8c19 
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
@ifed01 ifed01 force-pushed the wip-ifed-bitmap-alloc branch from 710006d to a8e8c19 Compare June 12, 2018 11:55
@ifed01
Copy link
Contributor Author

ifed01 commented Jun 12, 2018 •
edited
Loading

@ifed01 any concerns before we merge this?

@liewegas - nothing from my side.

@LenzGr LenzGr merged commit a8e8c19 into ceph:master Jun 12, 2018
liewegas added a commit that referenced this pull request Jun 12, 2018
@liewegas
Merge PR #21825 into master
3422e05 
* refs/pull/21825/head:
	os/bluestore: rename new bitmap allocator class to BitmapAllocator.
	os/bluestore: perform allocations aligned with min_length  in new bitmap allocator
	test/objectstore/unitetest_fastbmap_allocator: replace ASSERT_TRUE with
	os/bluestore: respect min_length as allocation granularity for new
	os/bluestore: cosmetic new allocator internal method rename.
	os/bluestore: properly respect min_length when analysing partially free
	os/bluestore: cosmetic cleanup in new bitmap allocator.
	os/bluestore: more verbose logging in new bitmap allocator
	os/bluestore: default to bitmap allocator for bluestore/bluefs
	os/bluestore: remove original bitmap allocator
	os/bluestore: align BitMap allocator's init_rm_free/init_add_free parameters with min_alloc_size
	os/bluestore: fix improper access to a BitmapFastAllocator::last_pos
	test/allocator: move bluestore allocator's benchmarks to a standalone UT
	os/bluestore: add new bitmap allocator
	test/allocator: get rid off bitmap allocator specifics and introduce new
	test/fio: dump mempool on job completion

Reviewed-by: Varada Kari <varadaraja.kari@flipkart.com>
@ifed01 ifed01 deleted the wip-ifed-bitmap-alloc branch June 12, 2018 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liewegas liewegas

@varadakari varadakari

@aclamk aclamk

Assignees
No one assigned
Labels
bluestore feature needs-review performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ifed01 @markhpc @liewegas @varadakari @LenzGr