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

osd: Add recovery sleep configuration option for HDDs and SSDs #16328

Merged
merged 3 commits into from Jul 21, 2017

Conversation

Projects
None yet
6 participants
@neha-ojha
Member

neha-ojha commented Jul 13, 2017

This pull request creates separate osd_recovery_sleep configuration option for HDDs and SSDs.

Signed-off-by: Neha Ojha nojha@redhat.com

@@ -816,6 +816,8 @@ OPTION(osd_op_thread_suicide_timeout, OPT_INT, 150)
OPTION(osd_recovery_thread_timeout, OPT_INT, 30)
OPTION(osd_recovery_thread_suicide_timeout, OPT_INT, 300)
OPTION(osd_recovery_sleep, OPT_FLOAT, 0.01) // seconds to sleep between recovery ops

This comment has been minimized.

@liewegas

liewegas Jul 13, 2017

Member

let's zero out the osd_recovery_sleep one, since that takes precedence.

This comment has been minimized.

@neha-ojha

neha-ojha Jul 13, 2017

Member

updated it!

This comment has been minimized.

@dzafman

dzafman Jul 13, 2017

Member

Actually with your change nobody is left using "osd_recovery_sleep," so shouldn't it be deleted from config options?

This comment has been minimized.

@liewegas

liewegas Jul 14, 2017

Member

It's kept around so that the admin can set it with the simple option name (and existing configs won't break). We do the same with all the other _ssd/_hdd options.

@liewegas

otherwise looks good!

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 13, 2017

Hmm, do we actually want to do

  • osd_recovery_sleep_filestore_hdd
  • osd_recovery_sleep_filestore_ssd
  • osd_recovery_sleep_bluestore_hdd
  • osd_recovery_sleep_bluestore_ssd

?

osd: Add recovery sleep configuration option for HDDs and SSDs
Signed-off-by: Neha Ojha <nojha@redhat.com>
@@ -815,7 +815,9 @@ OPTION(osd_op_thread_timeout, OPT_INT, 15)
OPTION(osd_op_thread_suicide_timeout, OPT_INT, 150)
OPTION(osd_recovery_thread_timeout, OPT_INT, 30)
OPTION(osd_recovery_thread_suicide_timeout, OPT_INT, 300)
OPTION(osd_recovery_sleep, OPT_FLOAT, 0.01) // seconds to sleep between recovery ops
OPTION(osd_recovery_sleep, OPT_FLOAT, 0) // seconds to sleep between recovery ops
OPTION(osd_recovery_sleep_hdd, OPT_FLOAT, 0.1)

This comment has been minimized.

@tchaikov

tchaikov Jul 14, 2017

Contributor

could you document these newly added options in doc/rados/configuration/osd-config-ref.rst?

This comment has been minimized.

@neha-ojha

@tchaikov tchaikov added the needs-doc label Jul 14, 2017

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Jul 14, 2017

@liewegas There do seem to be different optimal values for filestore and bluestore. Should we make the choice for the user, based on whether they are using filestore or bluestore? This will give fewer options to the end user and help keep the config options cleaner.

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 14, 2017

@tchaikov tchaikov removed the needs-doc label Jul 14, 2017

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Jul 14, 2017

retest this please

doc: add osd recovery sleep for HDDs and SSDs
Signed-off-by: Neha Ojha <nojha@redhat.com>

@liewegas liewegas changed the title from [DNM]osd: Add recovery sleep configuration option for HDDs and SSDs to osd: Add recovery sleep configuration option for HDDs and SSDs Jul 19, 2017

@liewegas liewegas added this to the luminous milestone Jul 19, 2017

@liewegas liewegas added the needs-qa label Jul 19, 2017

``osd recovery sleep hdd``
:Description: Time to sleep before next recovery for HDDs.

This comment has been minimized.

@jdurgin

jdurgin Jul 19, 2017

Member

perhaps clarify 'next recovery or backfill op' and 'in seconds'

This comment has been minimized.

@neha-ojha

neha-ojha Jul 19, 2017

Member

right, I'll update it for all three of them.

doc: update osd recovery sleep description
Signed-off-by: Neha Ojha <nojha@redhat.com>
@markhpc

This comment has been minimized.

Member

markhpc commented Jul 20, 2017

FWIW, it's possible the optimal bluestore values are going to change again if/when 16450 merges.

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Jul 20, 2017

@markhpc We haven't introduced separate optimal values based on store yet. It'd be interesting to see if with 16450, that might also be required or not.

@@ -2314,6 +2314,16 @@ int OSD::get_num_op_threads()
return get_num_op_shards() * cct->_conf->osd_op_num_threads_per_shard_ssd;
}
float OSD::get_osd_recovery_sleep()

This comment has been minimized.

@tchaikov

tchaikov Jul 21, 2017

Contributor

could you mark this method const? like

float OSD::get_osd_recovery_sleep() const

This comment has been minimized.

@neha-ojha

neha-ojha Jul 21, 2017

Member

@tchaikov yes, I am working further on this. Will it be fine if I change it in the next PR?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 21, 2017

@tchaikov tchaikov merged commit 41c5953 into ceph:master Jul 21, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment