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: small fixes in bluestore StupidAllocator #8101

Merged
merged 2 commits into from Mar 19, 2016

Conversation

jjhuo
Copy link
Contributor

@jjhuo jjhuo commented Mar 14, 2016

Main change is to avoid duplicated searches during allocating free extents.

@@ -98,6 +98,7 @@ int StupidAllocator::allocate(
int orig_bin = bin;

auto p = free[0].begin();
auto end = free[0].end();
Copy link
Contributor

Choose a reason for hiding this comment

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

better off moving the definition of end to where it is used. BTW, its value is always overwritten there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do.

@tchaikov
Copy link
Contributor

@jjhuo need to rebase.

@jjhuo jjhuo changed the title Two small bluestore fixes Small fixes in bluestore StupidAllocator Mar 17, 2016
@jjhuo
Copy link
Contributor Author

jjhuo commented Mar 17, 2016

@tchaikov rebase is done, thanks for help!

@@ -119,7 +118,7 @@ int StupidAllocator::allocate(
// search up (from origin, and skip searched extents by hint)
for (bin = orig_bin; bin < (int)free.size(); ++bin) {
p = free[bin].begin();
end = hint ? free[bin].lower_bound(hint) : free[bin].end();
auto end = hint ? free[bin].lower_bound(hint) : free[bin].end();
Copy link
Contributor

Choose a reason for hiding this comment

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

can you squash this change into the previous one? as we are not expecting the fix-the-fix commit in a single PR.

@tchaikov
Copy link
Contributor

maybe you need to remove the "// search down (from origin, and skip searched extents by hint)" change. probably we should have some test for verifying this behaviour.

When StupidAllocator searches for free extents, if hint is not 0(most times),
it will search extents starting from hint and up in each applicable bin,
if not found, then search all extents in those bins again.

If the extent to find(>= needs) exists below hint and is not in the choose_bin(),
let's say it's in the bin_X; then during this twice searches, all extents from
hint and up in bins before bin_X will be searched twice. This patch will eliminate
those unnecessary searches.

Signed-off-by: Jianjian Huo <samuel.huo@gmail.com>
Allocate() could return extent with size < need_size, so rename it per
Sage's suggestion.

Signed-off-by: Jianjian Huo <samuel.huo@gmail.com>
@jjhuo
Copy link
Contributor Author

jjhuo commented Mar 17, 2016

Finished the code squash.
And as Sage said, allocate() will search down and return smaller extents, that means this fix will skip those unnecessary searches as well. I have ran ceph_test_objectstore and saw same behaviors.

@liewegas
Copy link
Member

lgtm, thanks!

@liewegas liewegas changed the title Small fixes in bluestore StupidAllocator os/bluestore: small fixes in bluestore StupidAllocator Mar 19, 2016
liewegas added a commit that referenced this pull request Mar 19, 2016
os/bluestore: small fixes in bluestore StupidAllocator

Reviewed-by: Kefu Chai <kchai@redhat.com>
Reviewed-by: Sage Weil <sage@redhat.com>
@liewegas liewegas merged commit a10b059 into ceph:master Mar 19, 2016
@jjhuo jjhuo deleted the bluestore branch March 21, 2016 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants