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 bug in _open_alloc() #13577

Merged
merged 1 commit into from Mar 24, 2017

Conversation

Projects
None yet
5 participants
@yonghengdexin735
Contributor

yonghengdexin735 commented Feb 22, 2017

Here adding judgement for Allocator::create in _open_alloc(), bluestore_allocator may not equal stuqid or bitmap and then alloc = nullptr.

Signed-off-by: yonghengdexin735 zhang.zezhu@zte.com.cn

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
@@ -3760,6 +3760,10 @@ int BlueStore::_open_alloc()
alloc = Allocator::create(cct, cct->_conf->bluestore_allocator,
bdev->get_size(),
min_alloc_size);
if (alloc == nullptr)
lderr(cct) << "Allocator::create unknown alloc type" << cct->_conf->bluestore_allocator << dendl;
return -EINVAL;

This comment has been minimized.

@varadakari

varadakari Feb 22, 2017

Contributor

it has to be in { }

@varadakari

varadakari Feb 22, 2017

Contributor

it has to be in { }

This comment has been minimized.

@varadakari

varadakari Feb 22, 2017

Contributor

Shouldn't it be an assert?

@varadakari

varadakari Feb 22, 2017

Contributor

Shouldn't it be an assert?

This comment has been minimized.

@varadakari

varadakari Feb 22, 2017

Contributor

Or we should not allow any null value to bluestore_allocator or we should check only valid ones are entered in the conf file. That check better be before open_alloc()

@varadakari

varadakari Feb 22, 2017

Contributor

Or we should not allow any null value to bluestore_allocator or we should check only valid ones are entered in the conf file. That check better be before open_alloc()

This comment has been minimized.

@yonghengdexin735

yonghengdexin735 Feb 22, 2017

Contributor

thanks, I got it.

@yonghengdexin735

yonghengdexin735 Feb 22, 2017

Contributor

thanks, I got it.

This comment has been minimized.

@yonghengdexin735
@yonghengdexin735
@songbaisen

This comment has been minimized.

Show comment
Hide comment
@songbaisen

songbaisen Feb 23, 2017

jenkins test this please

songbaisen commented Feb 23, 2017

jenkins test this please

@songbaisen

This comment has been minimized.

Show comment
Hide comment
@songbaisen

songbaisen commented Feb 23, 2017

lgtm!

@yonghengdexin735

This comment has been minimized.

Show comment
Hide comment
@yonghengdexin735
Contributor

yonghengdexin735 commented Feb 23, 2017

@varadakari

lgtm

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
string type;
type = cct->_conf->bluestore_allocator;
assert(type == "stupid" || type == "bitmap");
alloc = Allocator::create(cct, type,

This comment has been minimized.

@tchaikov

tchaikov Feb 25, 2017

Contributor

could just check the return value of Allocator::create(), no need to check type beforehand.

@tchaikov

tchaikov Feb 25, 2017

Contributor

could just check the return value of Allocator::create(), no need to check type beforehand.

@tchaikov tchaikov added the core label Feb 25, 2017

@varadakari varadakari added bluestore and removed core labels Feb 25, 2017

@yonghengdexin735

This comment has been minimized.

Show comment
Hide comment
@yonghengdexin735
Contributor

yonghengdexin735 commented Feb 25, 2017

@tchaikov fixed

@yonghengdexin735

This comment has been minimized.

Show comment
Hide comment
@yonghengdexin735
Contributor

yonghengdexin735 commented Feb 27, 2017

@tchaikov done

@varadakari

This comment has been minimized.

Show comment
Hide comment
@varadakari

varadakari Mar 1, 2017

Contributor

LGTM

Contributor

varadakari commented Mar 1, 2017

LGTM

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
@@ -3760,6 +3760,11 @@ int BlueStore::_open_alloc()
alloc = Allocator::create(cct, cct->_conf->bluestore_allocator,
bdev->get_size(),
min_alloc_size);
if (alloc == nullptr) {
lderr(cct) << "Allocator::unknown alloc type" << cct->_conf->bluestore_allocator << dendl;

This comment has been minimized.

@tchaikov

tchaikov Mar 1, 2017

Contributor

please add a colon and a space after "type", and would be better to use __func__ here. like

if (!alloc) {
  lderr(cct) << __func__ << ": unknown bluestore_allocator: " << cct->_conf->bluestore_allocator << dendl;
}
@tchaikov

tchaikov Mar 1, 2017

Contributor

please add a colon and a space after "type", and would be better to use __func__ here. like

if (!alloc) {
  lderr(cct) << __func__ << ": unknown bluestore_allocator: " << cct->_conf->bluestore_allocator << dendl;
}

This comment has been minimized.

@tchaikov

tchaikov Mar 1, 2017

Contributor

and you can be more specific in your commit message title, "fix bug" is too vague a wording.

@tchaikov

tchaikov Mar 1, 2017

Contributor

and you can be more specific in your commit message title, "fix bug" is too vague a wording.

This comment has been minimized.

@yonghengdexin735

yonghengdexin735 Mar 2, 2017

Contributor

@tchaikov thanks, done

@yonghengdexin735

yonghengdexin735 Mar 2, 2017

Contributor

@tchaikov thanks, done

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
<< cct->_conf->bluestore_allocator
<< dendl;
return -EINVAL;
}

This comment has been minimized.

@tchaikov

tchaikov Mar 2, 2017

Contributor
if (!alloc) {
  lderr(cct) << __func__ << ": unknown bluestore_allocator: " << cct->_conf->bluestore_allocator << dendl;
}

could you test this patch, and check the output if the bluestore_allocator is not supported. to see why i'd suggest this change?

@tchaikov

tchaikov Mar 2, 2017

Contributor
if (!alloc) {
  lderr(cct) << __func__ << ": unknown bluestore_allocator: " << cct->_conf->bluestore_allocator << dendl;
}

could you test this patch, and check the output if the bluestore_allocator is not supported. to see why i'd suggest this change?

@liewegas liewegas changed the title from os/bluestore:fix bug in _open_alloc() to os/bluestore: fix bug in _open_alloc() Mar 15, 2017

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
@@ -3760,6 +3760,13 @@ int BlueStore::_open_alloc()
alloc = Allocator::create(cct, cct->_conf->bluestore_allocator,
bdev->get_size(),
min_alloc_size);
if (!alloc) {
lderr(cct) << __func__ << "Allocator::unknown alloc type"

This comment has been minimized.

@liewegas

liewegas Mar 15, 2017

Member

missing space before Allocator

@liewegas

liewegas Mar 15, 2017

Member

missing space before Allocator

This comment has been minimized.

@yonghengdexin735

yonghengdexin735 Mar 16, 2017

Contributor

@liewegas Thanks ,fixed

@yonghengdexin735

yonghengdexin735 Mar 16, 2017

Contributor

@liewegas Thanks ,fixed

os/bluestore:fixed bug for judging the return value in _open_alloc()
Signed-off-by: yonghengdexin735 <zhang.zezhu@zte.com.cn>

@liewegas liewegas added the needs-qa label Mar 16, 2017

@liewegas liewegas merged commit c820749 into ceph:master Mar 24, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment