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
librbd: fix don't send get_stripe_unit_count if striping is not enabled #17660
Conversation
src/librbd/image/OpenRequest.cc
Outdated
@@ -277,7 +277,10 @@ Context *OpenRequest<I>::handle_v2_get_immutable_metadata(int *result) { | |||
<< cpp_strerror(*result) << dendl; | |||
send_close_image(*result); | |||
} else { | |||
send_v2_get_stripe_unit_count(); | |||
if ((m_image_ctx->features & RBD_FEATURE_STRIPINGV2) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: what about if (m_image_ctx->features & RBD_FEATURE_STRIPINGV2)
like https://github.com/ceph/ceph/blob/master/src/tools/rbd/action/Info.cc#L261.
Otherwise, looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yangdongsheng :) done.
e2bde12
to
e905029
Compare
src/librbd/image/OpenRequest.cc
Outdated
@@ -277,7 +277,10 @@ Context *OpenRequest<I>::handle_v2_get_immutable_metadata(int *result) { | |||
<< cpp_strerror(*result) << dendl; | |||
send_close_image(*result); | |||
} else { | |||
send_v2_get_stripe_unit_count(); | |||
if (m_image_ctx->features & RBD_FEATURE_STRIPINGV2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmayyyha Wait, did you test this patch? Because the features would be applied in refresh step. so the features is not already set at this place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trociny @yangdongsheng fixed. I think calling get_stripe_unit_count
should be before init_layout rather than after refresh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the reason why we can't get striping information after refresh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey, after a look at init_layout() it's true.
e905029
to
b360948
Compare
@gmayyyha Agree, get_stripe_unit_count should be called before |
src/librbd/image/OpenRequest.cc
Outdated
} | ||
if (*result < 0) { | ||
lderr(cct) << "failed to retreive immutable metadata: " | ||
<< cpp_strerror(*result) << dendl; | ||
send_close_image(*result); | ||
} else { | ||
send_v2_get_stripe_unit_count(); | ||
if (m_image_ctx->features & RBD_FEATURE_STRIPINGV2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmayyyha I think using test_features()
is a bit cleaner.
@gmayyyha you can get the features by get_features(), example: But you have to refactor it into get_features_start() and get_features_finish(). I think. |
@gmayyyha Also note, you can't just modify an existing cls function to retrieve So, I think you either need to add @dillaman what do you think? |
b360948
to
e702a0e
Compare
@yangdongsheng @trociny Thanks for your suggestions. I add |
retest this please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nicer to add the get_features
cls call to the existing cls_client::get_immutable_metadata
call to avoid the second round-trip call to the OSDs. In RefreshRequest
, the reason why we have to many steps is only due to backwards compatibility -- which wouldn't be an issue here.
src/librbd/image/OpenRequest.cc
Outdated
@@ -277,7 +277,48 @@ Context *OpenRequest<I>::handle_v2_get_immutable_metadata(int *result) { | |||
<< cpp_strerror(*result) << dendl; | |||
send_close_image(*result); | |||
} else { | |||
send_v2_get_stripe_unit_count(); | |||
send_v2_get_features(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: spacing
src/librbd/image/OpenRequest.cc
Outdated
lderr(cct) << "failed to retreive features: " | ||
<< cpp_strerror(*result) << dendl; | ||
send_close_image(*result); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: just return nullptr
if you need to close the image and then eliminate the nested if
block
@trociny We can always rename the method in |
e702a0e
to
f93e5f1
Compare
@gmayyyha I didn't see the first version, so I cannot comment. I would add it to |
+1 |
f93e5f1
to
859f699
Compare
@dillaman when I try to enable [root@ceph14 build]# rbd feature enable disk01 striping
2017-09-14 22:19:14.377140 7f5ec3b1ed40 -1 WARNING: all dangerous and experimental features are enabled.
2017-09-14 22:19:14.378398 7f5ec3b1ed40 -1 WARNING: all dangerous and experimental features are enabled.
2017-09-14 22:19:14.426221 7f5ec3b1ed40 -1 WARNING: all dangerous and experimental features are enabled.
rbd: failed to update image features: 2017-09-14 22:19:14.534979 7f5ec3b1ed40 -1 librbd::Operations: cannot update immutable features
(22) Invalid argument |
@gmayyyha The |
@gmayyyha Jason suggested to rename it to |
Also, if you want this in 2 commits, it is more logical to have in the first commit |
0aa2a00
to
3c1ad3d
Compare
@trociny all done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
try { | ||
uint64_t size; | ||
// get_size | ||
::decode(*order, *it); | ||
::decode(size, *it); | ||
// get_object_prefix | ||
::decode(*object_prefix, *it); | ||
// get_features | ||
::decode(*features, *it); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, get_features
returns also incompatible_features
. It is not necessary to decode this field here, because it is the last item. On the other hand, if someone later adds yet another get_xyz
after get_features
he might forget (not know) to decode incompatible_features
, so probably it makes sense to decode it (or left a comment). I don't have a strong opinion here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for decoding it into a throwaway local variable for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks like this comment is open
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dillaman decode features
for test_features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmayyyha We meant that you need to add something like this:
uint64_t incompatible_features;
::decode(incompatible_features, *it);
See "get_size" case above where we decode size in a throwaway local variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trociny Ah... Sorry, I misunderstand. fixed.
src/cls/rbd/cls_rbd_client.h
Outdated
void get_immutable_metadata_start(librados::ObjectReadOperation *op); | ||
int get_immutable_metadata_finish(bufferlist::iterator *it, | ||
void get_initial_metadata_start(librados::ObjectReadOperation *op); | ||
int get_initial_metadata_finish(bufferlist::iterator *it, | ||
std::string *object_prefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: adjust spacing to match alignment
3c1ad3d
to
10dd165
Compare
retest this please |
10dd165
to
2ca15b9
Compare
2ca15b9
to
0d0d01f
Compare
Fixes: http://tracker.ceph.com/issues/21360 Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>
0d0d01f
to
3995fe2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
…ot enabled by gmayyyha · Pull Request ceph#17660 · ceph/ceph · GitHub * striping-feature-21360: librbd: fix don't send get_stripe_unit_count if striping is not enabled
Fixes: http://tracker.ceph.com/issues/21360
Signed-off-by: Yanhu Cao gmayyyha@gmail.com