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 reclaim_blocks and clean up Allocator interface #12963

Merged
merged 8 commits into from Jan 19, 2017

Conversation

Projects
None yet
3 participants
@liewegas
Member

liewegas commented Jan 17, 2017

This evolved into something truly ugly, and reclaim_blocks managed to misuse
it. Fix and clean.

@liewegas liewegas requested review from ifed01 and varadakari Jan 17, 2017

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas
Member

liewegas commented Jan 17, 2017

@chhabaramesh

This comment has been minimized.

Show comment
Hide comment
@chhabaramesh

chhabaramesh Jan 17, 2017

Contributor

@liewegas , overall changes looks good. Only thing is if emblace_back could cause multiple allocations in single allocator call? are we worried about that?

Contributor

chhabaramesh commented Jan 17, 2017

@liewegas , overall changes looks good. Only thing is if emblace_back could cause multiple allocations in single allocator call? are we worried about that?

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jan 17, 2017

Member

We can do a reserve() call in the caller for that if we want. I suspect that a small number like 2 or 4 is sufficient.

Member

liewegas commented Jan 17, 2017

We can do a reserve() call in the caller for that if we want. I suspect that a small number like 2 or 4 is sufficient.

@chhabaramesh

This comment has been minimized.

Show comment
Hide comment
@chhabaramesh

chhabaramesh Jan 17, 2017

Contributor

yes, I think that would suffice since most of allocations are single to few blocks.

Contributor

chhabaramesh commented Jan 17, 2017

yes, I think that would suffice since most of allocations are single to few blocks.

@liewegas liewegas added the needs-qa label Jan 17, 2017

@chhabaramesh

Changes looks good to me.

Show outdated Hide outdated src/os/bluestore/BlueFS.cc Outdated
@ifed01

ifed01 approved these changes Jan 18, 2017

LGTM except some nits

liewegas added some commits Jan 17, 2017

os/bluestore: return blocks allocated from allocate()
Instead of having a separate output argument with the number of
blocks allocated, just return it via the return value.  Simplifies
the calling convention.

Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: drop useless count arg to allocate
The vector<> has a size.

Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore/Allocator: drop unused and goofy release_extents
Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: manage vector from ExtentList
ExtentList was previous relying the caller to preallocate/size the
vector to be large enough for the worst case allocation of extents,
and keeping it's own manual count of the extent list size.  Instead,
manage that from ExtentList, and remove the preallocation from the
callers.

Fixes: http://tracker.ceph.com/issues/18573
Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: prealloc/reserve extent vector for common alloc path
No need to worry about the gift/reclaim path--those are very rare.

Signed-off-by: Sage Weil <sage@redhat.com>
unitest_bit_alloc, unittest_alloc: fixes
Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore/BlueFS: handle failure to reclaim blocks without crashing
We shouldn't fail to reclaim space in general, but if we do, do not treat
it as a fatal error.  Log loudly.

Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore/BlueFS: dump allocator freelist on failure in reclaim_bl…
…ocks

Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas merged commit 04980b7 into ceph:master Jan 19, 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

@liewegas liewegas deleted the liewegas:wip-bluestore-extents branch Jan 19, 2017

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