librbd: default features should be negotiated with the OSD #11808
Conversation
|
@dillaman Not sure about util::get_rbd_default_features(). I mostly need it for internal.cc, but it will not work for a new pool, because of RBD_DIRECTORY not existing yet. |
| @@ -89,7 +91,7 @@ uint64_t parse_rbd_default_features(CephContext* cct) | |||
| value += conf_vals[feature]; | |||
| } else { | |||
| ret = -EINVAL; | |||
| ldout(cct, 1) << "Warning: unknown rbd feature " << feature << dendl; | |||
| lderr(cct) << "unknown rbd feature " << feature << dendl; | |||
There was a problem hiding this comment.
Note that this falls back to the defaults on -EINVAL below. That was the rationale for the warning since execution will continue.
There was a problem hiding this comment.
Yes, I am aware that execution will continue, but I think it is better to display it as an error message to the user (though not fatal). Without this and with rbd debug = 0 by default, the user will not see e.g. that she has a typo in ceph.conf.
There was a problem hiding this comment.
Makes sense. Perhaps reword it to be "ignoring unknown feature ..."
|
@trociny I was thinking that this negotiation would only occur if using the default image features (i.e. the user didn't supply any features when creating the image). If the user supplied a feature not supported by the server, I'd say let it fail. In terms of the existence of rbd_directory, if this check could be added to the create state machine after a (modified) validate pool step where you would create the directory if it didn't exist (if pool validation was disabled). |
There was a problem hiding this comment.
@dillaman I think I implemented it exactly as you was thinking (only if no features are provided and in create state machine after validate pool step). The problem is only with one (compatibility?) case, for create() function which no features parameters, when parse_rbd_default_features was used. Probably I could just use features = 0 in that case, I just don't analyze yet if it will break something.
| @@ -89,7 +91,7 @@ uint64_t parse_rbd_default_features(CephContext* cct) | |||
| value += conf_vals[feature]; | |||
| } else { | |||
| ret = -EINVAL; | |||
| ldout(cct, 1) << "Warning: unknown rbd feature " << feature << dendl; | |||
| lderr(cct) << "unknown rbd feature " << feature << dendl; | |||
There was a problem hiding this comment.
Yes, I am aware that execution will continue, but I think it is better to display it as an error message to the user (though not fatal). Without this and with rbd debug = 0 by default, the user will not see e.g. that she has a typo in ceph.conf.
| bufferlist::iterator it = m_outbl.begin(); | ||
| *result = cls_client::get_all_features_finish(&it, &all_features); | ||
| } | ||
| if (*result < 0) { |
There was a problem hiding this comment.
Minor: probably good to check for -EOPNOTSUPP and skip the negotiation if it fails.
There was a problem hiding this comment.
@dillaman Not sure about this. It is only a debug message, so it looks like it does not hurt to display it even for EOPNOTSUPP case.
There was a problem hiding this comment.
Sorry, late night last night -- indeed, never mind.
| if (*result < 0) { | ||
| ldout(m_cct, 10) << "error retrieving server supported features set: " | ||
| << cpp_strerror(*result) << dendl; | ||
| } else if (all_features < m_features) { |
There was a problem hiding this comment.
Minor: if ((m_features & all_features) != m_features) is probably clearer
|
@trociny For the create() w/ no features case, you could just have the it call the create method that takes the ImageOptions instead of that intermediate version for stripe units. |
|
@dillaman good idea! Thanks for your comments. |
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
Fixes: http://tracker.ceph.com/issues/17010 Signed-off-by: Mykola Golub <mgolub@mirantis.com>
|
@dillaman updated |
|
lgtm |
Fixes: http://tracker.ceph.com/issues/17010