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/filestore/HashIndex: randomize split threshold by a configurable amount #15689

Merged
merged 1 commit into from Jul 7, 2017

Conversation

Projects
None yet
7 participants
@jdurgin
Member

jdurgin commented Jun 14, 2017

Store a random value up to the filestore_split_rand_factor for each
collection when it is created or apply-layout-settings is run. This
should help distribute the load of splitting directories across a
longer period of time.

Fixes: http://tracker.ceph.com/issues/15835
Signed-off-by: Josh Durgin jdurgin@redhat.com

@jdurgin jdurgin requested a review from liewegas Jun 14, 2017

@@ -1308,6 +1308,7 @@ OPTION(filestore_commit_timeout, OPT_FLOAT, 600)
OPTION(filestore_fiemap_threshold, OPT_INT, 4096)
OPTION(filestore_merge_threshold, OPT_INT, 10)
OPTION(filestore_split_multiple, OPT_INT, 2)
OPTION(filestore_split_rand_factor, OPT_U32, 0) // randomize the split threshold by adding 16 * [0, rand_factor)

This comment has been minimized.

@liewegas

liewegas Jun 14, 2017

Member

we should enable this by default, right?

This comment has been minimized.

@jdurgin

jdurgin Jun 14, 2017

Member

I suppose it can't hurt. We just don't have a great number to put here without good perf testing

@liewegas liewegas changed the title from HashIndex: randomize split threshold by a configurable amount to os/filestore/HashIndex: randomize split threshold by a configurable amount Jun 14, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 14, 2017

default aside, this looks good!

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 14, 2017

@@ -302,7 +302,7 @@ Misc
``filestore split multiple``
:Description: ``filestore_split_multiple * abs(filestore_merge_threshold) * 16``
:Description: ``(filestore_split_multiple * abs(filestore_merge_threshold) + (rand() % filestore_split_rand)) * 16``

This comment has been minimized.

@xiexingguo

xiexingguo Jun 15, 2017

Member

s/filestore_split_rand/filestore_split_rand_factor/

btw, it's a little weird to me that we have both options with split prefix and merge prefix here...

This comment has been minimized.

@jdurgin

jdurgin Jun 15, 2017

Member

good catch. yeah, these aren't the most intuitive options, but at this point I'd like to change filestore as little as possible

This comment has been minimized.

@xiexingguo

xiexingguo Jun 15, 2017

Member

but at this point I'd like to change filestore as little as possible

sure:-)

HashIndex: randomize split threshold by a configurable amount
Store a random value up to the filestore_split_rand_factor for each
collection when it is created or apply-layout-settings is run. This
should help distribute the load of splitting directories across a
longer period of time.

Fixes: http://tracker.ceph.com/issues/15835
Signed-off-by: Josh Durgin <jdurgin@redhat.com>

@tchaikov tchaikov self-requested a review Jun 15, 2017

@markhpc

This comment has been minimized.

Member

markhpc commented Jun 20, 2017

How much we randomize this sort of depends on the total numbers of PGs, the number of PGs per OSD, the time it takes to perform a split, and how much variance in count there is when psuedo-randomly distributing objects into PGs. I suspect that 10-20% is a good value. It may be that depending on the exact situation where more or less would be better, but for now I think that's a good place to start.

@jdurgin jdurgin merged commit 5cc8921 into ceph:master Jul 7, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details
@ygtzf

This comment has been minimized.

Contributor

ygtzf commented Jul 10, 2017

@jdurgin
There seems to be a clerical error on line 47 of file src/os/filestore/HashIndex.h:

16 * (abs(merge_threshhold)) * split_multiplier +

The last ')' should be removed.

Thanks.

@jdurgin jdurgin deleted the jdurgin:wip-filestore-rand-split branch Jul 12, 2017

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