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: Check whether journal is rotational or not #16614

Merged
merged 4 commits into from Aug 4, 2017

Conversation

Projects
None yet
4 participants
@neha-ojha
Member

neha-ojha commented Jul 26, 2017

This feature is required before we can introduce the osd_recovery_sleep_hybrid configuration option.
It creates is_journal_rotational() method for FileStore and BlueStore, which checks whether the journal is on a rotational device or not.

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

@neha-ojha neha-ojha requested review from liewegas and jdurgin Jul 26, 2017

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Jul 26, 2017

retest this please

@liewegas

filestore bits look good, but i'm 90% sure the bluestore case is only called after mount

@@ -4395,6 +4411,31 @@ bool BlueStore::is_rotational()
return rotational;
}
bool BlueStore::is_journal_rotational()
{

This comment has been minimized.

@liewegas

liewegas Jul 27, 2017

Member

assert(bluefs);
return bluefs->wal_is_rotational();

and drop the rest of this method... pretty sure we can ignore the case where this gets called before mount()?

This comment has been minimized.

@liewegas

liewegas Jul 27, 2017

Member

The bluefs impl willb e somethhing like

if (!bdev[BDEV_WAL] || bdev[BDEV_WAL]->is_rotational()) return true;
return false;

This comment has been minimized.

@neha-ojha

neha-ojha Jul 27, 2017

Member

@liewegas I used the above implementation and I am hitting FAILED assert(bluefs).
Looking at logs, I noticed that bluestore umount() and bluefs umount() are being called before BlueStore::is_journal_rotational(). I'll dig into this further to figure out what's going on. Any ideas?

This comment has been minimized.

@liewegas

liewegas Jul 27, 2017

Member

can you push what you currently have?

This comment has been minimized.

@neha-ojha

neha-ojha Jul 27, 2017

Member

just pushed

bool BlueFS::wal_is_rotational()
{
if (!bdev[BDEV_WAL] || bdev[BDEV_WAL]->is_rotational())

This comment has been minimized.

@jdurgin

jdurgin Jul 31, 2017

Member

wouldn't the all-ssd case satisfy the first condition? seems like we'd need to check the main device as well in that case

This comment has been minimized.

@neha-ojha

neha-ojha Jul 31, 2017

Member

@jdurgin This check is only for the journal. We will be checking both is_rotational and is_journal_rotational in the osd, before deciding which value of recovery sleep to use. I will be adding that logic in my next commit. Does that sound reasonable?

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Aug 1, 2017

retest this please

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Aug 1, 2017

@liewegas This is ready for review.

@liewegas liewegas changed the title from [DNM] osd: Check whether journal is rotational or not to osd: Check whether journal is rotational or not Aug 1, 2017

@liewegas liewegas added this to the luminous milestone Aug 1, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Aug 2, 2017

rebase please

neha-ojha added some commits Jul 26, 2017

osd: Check whether journal is rotational or not
Signed-off-by: Neha Ojha <nojha@redhat.com>
osd: Add recovery sleep hybrid configuration option
Signed-off-by: Neha Ojha <nojha@redhat.com>
doc: add documentation for osd_recovery_sleep_hybrid
Signed-off-by: Neha Ojha <nojha@redhat.com>
@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Aug 2, 2017

@liewegas Rebased. The make check failure looks unrelated.

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Aug 2, 2017

retest this please

1 similar comment
@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Aug 3, 2017

retest this please

return cct->_conf->osd_recovery_sleep_hdd;
else if (store_is_rotational && !journal_is_rotational)
return cct->_conf->get_val<double>("osd_recovery_sleep_hybrid");
else
return cct->_conf->osd_recovery_sleep_ssd;

This comment has been minimized.

@liewegas

liewegas Aug 4, 2017

Member

Hmm, can we flip the ssd and hdd cases around so that the (unlikely but possible) ssd store + hdd journal case is classified as 'hdd'?

@yuriw

This comment has been minimized.

Contributor

yuriw commented Aug 4, 2017

test this please

osd: use recovery sleep hdd for ssd store and hdd journal case
Signed-off-by: Neha Ojha <nojha@redhat.com>

@liewegas liewegas merged commit 6a39c5a into ceph:master Aug 4, 2017

2 of 4 checks passed

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