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.

Sign up for GitHub

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

Jump to bottom

osd/PrimaryLogPG: derr when object size becomes over osd_max_object_size #19049

Merged
merged 1 commit into from Dec 29, 2017
Merged

osd/PrimaryLogPG: derr when object size becomes over osd_max_object_size #19049

merged 1 commit into from Dec 29, 2017

Conversation

shinobu-x
Copy link
Contributor

Signed-off-by: Shinobu Kinjo shinobu@redhat.com

gregsfortytwo
gregsfortytwo requested changes Nov 21, 2017
View reviewed changes
src/osd/PrimaryLogPG.cc Outdated
if (result < 0) {
derr << __func__
<< " osd_max_object_size >= 4GB; BlueStore has hard limit of 4GB."
<< dendl;
Copy link
Member

Choose a reason for hiding this comment

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

How about moving the error logging into check_offset_and_length() instead of duplicating it everywhere that function is called?

src/osd/PrimaryLogPG.cc Outdated
@@ -5862,7 +5879,10 @@ int PrimaryLogPG::do_osd_ops(OpContext *ctx, vector<OSDOp>& ops)
break;
}

if (op.extent.offset > cct->_conf->osd_max_object_size) {
if (op.extent.offset > osd_max_object_size) {
Copy link
Member

Choose a reason for hiding this comment

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

Might as well swap this to use check_offset_and_length() while you're touching it.

src/osd/PrimaryLogPG.cc Outdated
osd_max_object_size);
if (result < 0) {
derr << __func__
<< " osd_max_object_size >= 4GB; BlueStore has hard limit of 4GB."
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't reference BlueStore or a constant hard limit in the OSD error code (they might be using FileStore!)
Just say it exceeds the max size.

@shinobu-x
Copy link
Contributor Author

@gregsfortytwo Please check when you get a chance.

@gregsfortytwo
Copy link
Member

That looks better; thanks.

tchaikov
tchaikov requested changes Nov 22, 2017
View reviewed changes
src/osd/PrimaryLogPG.cc Outdated
@@ -4245,8 +4245,11 @@ static int check_offset_and_length(uint64_t offset, uint64_t length, uint64_t ma
{
if (offset >= max ||
length > max ||
offset + length > max)
offset + length > max) {
std::cerr << __func__ << " "
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think the stderr is available after the ceph-osd is daemonized. please use ldpp_dout() for printing the log messages in a static function.

src/osd/PrimaryLogPG.cc Outdated
@@ -5862,8 +5871,9 @@ int PrimaryLogPG::do_osd_ops(OpContext *ctx, vector<OSDOp>& ops)
break;
}

if (op.extent.offset > cct->_conf->osd_max_object_size) {
result = -EFBIG;
if (op.extent.offset > osd_max_object_size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do this check twice? we already know that the the op will end up with an over-sized object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov Well, this block is already there. I don't think I added it newly. See:
https://github.com/ceph/ceph/blob/master/src/osd/PrimaryLogPG.cc#L5865-L5868

Copy link
Contributor

Choose a reason for hiding this comment

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

i mean, you could drop this line, if you choose to use the helper function. otherwise, we do the check twice after this change.

@shinobu-x shinobu-x force-pushed the primarylogpg_warning branch 2 times, most recently from 09eebe6 to 7881e91 Compare November 22, 2017 17:13
@shinobu-x
Copy link
Contributor Author

@tchaikov Please check when you get a chance.

tchaikov
tchaikov requested changes Nov 23, 2017
View reviewed changes
src/osd/PrimaryLogPG.cc Outdated
@@ -4927,6 +4932,9 @@ int PrimaryLogPG::do_osd_ops(OpContext *ctx, vector<OSDOp>& ops)
const hobject_t& soid = oi.soid;
bool skip_data_digest = osd->store->has_builtin_csum() &&
g_conf->get_val<bool>("osd_skip_data_digest");
auto osd_max_object_size = cct->_conf->get_val<uint64_t>(
"osd_max_object_size");
DoutPrefixProvider *dpp;
Copy link
Contributor

Choose a reason for hiding this comment

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

this variable is not initialized.

src/osd/PrimaryLogPG.cc Outdated
@@ -5700,7 +5708,8 @@ int PrimaryLogPG::do_osd_ops(OpContext *ctx, vector<OSDOp>& ops)
oi.truncate_size = op.extent.truncate_size;
}
}
result = check_offset_and_length(op.extent.offset, op.extent.length, cct->_conf->osd_max_object_size);
result = check_offset_and_length(op.extent.offset, op.extent.length,
osd_max_object_size, dpp);
Copy link
Contributor

Choose a reason for hiding this comment

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

use get_dpp() please.

@shinobu-x
Copy link
Contributor Author

@tchaikov Please check if you get a chance.

tchaikov
tchaikov reviewed Nov 28, 2017
View reviewed changes
src/osd/PrimaryLogPG.cc Outdated
result = check_offset_and_length(op.extent.offset, op.extent.length, cct->_conf->osd_max_object_size);
auto dpp = get_dpp();
result = check_offset_and_length(op.extent.offset, op.extent.length,
osd_max_object_size, dpp);
Copy link
Contributor

Choose a reason for hiding this comment

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

@shinobu-x instead of having a dpp local variable, i'd recommend just use get_dpp() directly.

@shinobu-x shinobu-x force-pushed the primarylogpg_warning branch 2 times, most recently from 1840c10 to 8448c67 Compare November 28, 2017 09:20
@shinobu-x
osd/PrimaryLogPG: derr when object size becomes over osd_max_object_size
e0b6b17 
Signed-off-by: Shinobu Kinjo <shinobu@redhat.com>
tchaikov
tchaikov reviewed Nov 29, 2017
View reviewed changes
src/osd/PrimaryLogPG.cc
offset + length > max)
offset + length > max) {
ldpp_dout(dpp, 10) << __func__ << " "
<< "osd_max_object_size >= 4GB; Hard limit of object size is 4GB.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to print the trailing \n.
nit, you are hardwiring the max to 4GB, but it is configurable.

@tchaikov
Copy link
Contributor

tchaikov commented Dec 1, 2017

@liewegas liewegas merged commit f8b6709 into ceph:master Dec 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

@gregsfortytwo gregsfortytwo gregsfortytwo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@shinobu-x @gregsfortytwo @tchaikov @liewegas