librbd: fix don't send get_stripe_unit_count if striping is not enabled by gmayyyha · Pull Request #17660 · ceph/ceph

gmayyyha · 2017-09-12T02:46:07Z

Fixes: http://tracker.ceph.com/issues/21360

Signed-off-by: Yanhu Cao gmayyyha@gmail.com

yangdongsheng · 2017-09-13T02:42:56Z

src/librbd/image/OpenRequest.cc

    send_close_image(*result);
  } else {
-    send_v2_get_stripe_unit_count();
+    if ((m_image_ctx->features & RBD_FEATURE_STRIPINGV2) != 0)


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.

@yangdongsheng :) done.

yangdongsheng · 2017-09-13T10:13:23Z

src/librbd/image/OpenRequest.cc

    send_close_image(*result);
  } else {
-    send_v2_get_stripe_unit_count();
+    if (m_image_ctx->features & RBD_FEATURE_STRIPINGV2)


@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.

@trociny @yangdongsheng fixed. I think calling get_stripe_unit_count should be before init_layout rather than after refresh

Could you explain the reason why we can't get striping information after refresh?

Okey, after a look at init_layout() it's true.

trociny · 2017-09-14T07:48:32Z

@gmayyyha Agree, get_stripe_unit_count should be called before m_image_ctx->init_layout() (I missed this in my PR). Though I don't like you modified get_immutable_metadata to retrieve features. Note, features is not "immutable" metadata.

trociny · 2017-09-14T07:50:29Z

src/librbd/image/OpenRequest.cc

    send_close_image(*result);
  } else {
-    send_v2_get_stripe_unit_count();
+    if (m_image_ctx->features & RBD_FEATURE_STRIPINGV2)


@gmayyyha I think using test_features() is a bit cleaner.

yangdongsheng · 2017-09-14T07:59:51Z

@gmayyyha you can get the features by get_features(), example:
https://github.com/ceph/ceph/blob/master/src/librbd/api/Mirror.cc#L582

But you have to refactor it into get_features_start() and get_features_finish(). I think.

trociny · 2017-09-14T08:17:11Z

@gmayyyha Also note, you can't just modify an existing cls function to retrieve features, because it will fail when a new client is used with older OSDs.

So, I think you either need to add get_features call before get_stripe_unit_count, or try to move m_image_ctx->init_layout() to be called after refresh (and get_stripe_unit_count) -- at first glance it looks like it should work.

@dillaman what do you think?

gmayyyha · 2017-09-14T12:09:34Z

@yangdongsheng @trociny Thanks for your suggestions. I add get_features call before 'get_stripe_unit_count'. and don't need to refactor get_features(https://github.com/ceph/ceph/blob/master/src/cls/rbd/cls_rbd_client.cc#L176) .

gmayyyha · 2017-09-14T12:35:28Z

retest this please.

dillaman

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.

dillaman · 2017-09-14T13:19:00Z

src/librbd/image/OpenRequest.cc

    send_close_image(*result);
  } else {
-    send_v2_get_stripe_unit_count();
+      send_v2_get_features();


Nit: spacing

dillaman · 2017-09-14T13:20:27Z

src/librbd/image/OpenRequest.cc

+    lderr(cct) << "failed to retreive features: "
+               << cpp_strerror(*result) << dendl;
+    send_close_image(*result);
+  } else {


Nit: just return nullptr if you need to close the image and then eliminate the nested if block

trociny · 2017-09-14T13:35:58Z

@dillaman Previously I wrote I didn't like the idea to return features in cls_client::get_immutable_metadata because features is "mutable". But if you like it I don't mind and @gmayyyha then sorry, and just make sure you do this in cls client, not server, as it was in your first version.

dillaman · 2017-09-14T13:37:47Z

@trociny We can always rename the method in cls_client to something like get_initial_metadata

gmayyyha · 2017-09-14T13:52:20Z

@trociny @dillaman revert to my first version?

dillaman · 2017-09-14T13:58:03Z

@gmayyyha I didn't see the first version, so I cannot comment. I would add it to cls_client::get_immutable_metadata and rename the function since it's doing more than retrieving immutable things now.

trociny · 2017-09-14T13:58:59Z

+1

gmayyyha · 2017-09-14T14:40:33Z

@dillaman when I try to enable RBD_FEATURE_STRIPINGV2. the output is immutable features. need to rename the function ?

[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

dillaman · 2017-09-14T16:27:45Z

@gmayyyha The RBD_FEATURE_STRIPINGV2 feature is immutable but other features are not. Therefore, the value returned from get_features can change / is not immutable.

trociny · 2017-09-15T06:56:38Z

@gmayyyha Jason suggested to rename it to get_initial_metadata. Also, when changing send_v2_get_immutable_metadata method name, don't forget to update ascii art state diagram in OpenRequest.h.

trociny · 2017-09-15T07:01:50Z

Also, if you want this in 2 commits, it is more logical to have in the first commit get_immutable_metadata rename + change to retrieve features, and in the second commit the actual fix (test for feature and skip get_stripe_unit_count if no striping).

gmayyyha · 2017-09-15T07:41:40Z

@trociny all done.

trociny

LGTM

trociny · 2017-09-15T09:25:55Z

src/cls/rbd/cls_rbd_client.cc

 	// get_object_prefix
 	::decode(*object_prefix, *it);
+	// get_features
+	::decode(*features, *it);


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.

I'd vote for decoding it into a throwaway local variable for clarity

Still looks like this comment is open

@dillaman decode features for test_features.

@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.

@trociny Ah... Sorry, I misunderstand. fixed.

dillaman · 2017-09-15T15:19:36Z

src/cls/rbd/cls_rbd_client.h

-    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,


Nit: adjust spacing to match alignment

gmayyyha · 2017-09-18T00:47:57Z

retest this please

Fixes: http://tracker.ceph.com/issues/21360 Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>

dillaman

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

liewegas added the rbd label Sep 12, 2017

yangdongsheng reviewed Sep 13, 2017

View reviewed changes

gmayyyha force-pushed the striping-feature-21360 branch from e2bde12 to e905029 Compare September 13, 2017 03:29

yangdongsheng mentioned this pull request Sep 13, 2017

librbd: call get_stripe_unit_count only if striping enabled #17693

Closed

yangdongsheng reviewed Sep 13, 2017

View reviewed changes

trociny added the cleanup label Sep 13, 2017

gmayyyha force-pushed the striping-feature-21360 branch from e905029 to b360948 Compare September 14, 2017 02:32

trociny reviewed Sep 14, 2017

View reviewed changes

gmayyyha force-pushed the striping-feature-21360 branch from b360948 to e702a0e Compare September 14, 2017 11:35

dillaman reviewed Sep 14, 2017

View reviewed changes

gmayyyha force-pushed the striping-feature-21360 branch from e702a0e to f93e5f1 Compare September 14, 2017 13:48

gmayyyha force-pushed the striping-feature-21360 branch from f93e5f1 to 859f699 Compare September 14, 2017 14:27

gmayyyha force-pushed the striping-feature-21360 branch from 0aa2a00 to 3c1ad3d Compare September 15, 2017 07:39

trociny approved these changes Sep 15, 2017

View reviewed changes

dillaman reviewed Sep 15, 2017

View reviewed changes

gmayyyha force-pushed the striping-feature-21360 branch from 3c1ad3d to 10dd165 Compare September 16, 2017 04:52

gmayyyha force-pushed the striping-feature-21360 branch from 10dd165 to 2ca15b9 Compare September 18, 2017 02:18

gmayyyha force-pushed the striping-feature-21360 branch from 2ca15b9 to 0d0d01f Compare September 25, 2017 07:07

librbd: fix don't send get_stripe_unit_count if striping is not enabled

3995fe2

Fixes: http://tracker.ceph.com/issues/21360 Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>

gmayyyha force-pushed the striping-feature-21360 branch from 0d0d01f to 3995fe2 Compare September 25, 2017 08:24

dillaman approved these changes Sep 25, 2017

View reviewed changes

dillaman added the needs-qa label Sep 25, 2017

trociny added the wip-mgolub-testing label Sep 26, 2017

dillaman added the wip-jason-testing label Oct 2, 2017

dillaman merged commit 24acc55 into ceph:master Oct 3, 2017

gmayyyha deleted the striping-feature-21360 branch October 9, 2017 02:01

Conversation

gmayyyha commented Sep 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trociny commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangdongsheng commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trociny commented Sep 14, 2017

Uh oh!

gmayyyha commented Sep 14, 2017

Uh oh!

gmayyyha commented Sep 14, 2017

Uh oh!

dillaman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trociny commented Sep 14, 2017

Uh oh!

dillaman commented Sep 14, 2017

Uh oh!

gmayyyha commented Sep 14, 2017

Uh oh!

dillaman commented Sep 14, 2017

Uh oh!

trociny commented Sep 14, 2017

Uh oh!

gmayyyha commented Sep 14, 2017

Uh oh!

dillaman commented Sep 14, 2017

Uh oh!

trociny commented Sep 15, 2017

Uh oh!

trociny commented Sep 15, 2017

Uh oh!

gmayyyha commented Sep 15, 2017

Uh oh!

trociny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmayyyha commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dillaman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

yangdongsheng commented Sep 14, 2017 •

edited

Loading

gmayyyha commented Sep 18, 2017 •

edited

Loading