messages/MForward: reencode forwarded message if target has differing features#11610
messages/MForward: reencode forwarded message if target has differing features#11610liewegas merged 3 commits intoceph:masterfrom
Conversation
|
adding this to http://tracker.ceph.com/issues/17386 also, it should be backported to jewel along with #11596. |
src/messages/MForward.h
Outdated
| // message are changed when reencoding with more features than the | ||
| // client had originally. That should never happen, but we may as | ||
| // well be defensive here. | ||
| if (CEPH_FEATURES_SUPPORTED_DEFAULT != features || |
There was a problem hiding this comment.
assuming we have three parties: osd.0 <=> mon.1 <=> mon.0. in which, osd.0 is connected to mon.1, which is a peon, and mon.0 is the leader of the quorum.
- osd.0 sends a request to mon.1, and mon.1 forwards it to mon.0
the
featureshere ismon[0].features & mon[1].features. and themsg->payloadis encoded usingcon_features(i.e.osd[0].features & mon[1].features).
in this patch we re-encode the request before forwarding it to mon.0. i think should always re-encode the wrapped request as long ascon_features != features, regardless ifmon[0].featuresis equal tomon[1].featureor not.
- mon.0 sends the reply of this message to mon.1, and mon.1 routes it to osd.0
ahh, we always
clear_payload(), okay then.
There was a problem hiding this comment.
in short, i don't think we need to check "CEPH_FEATURES_SUPPORTED_DEFAULT != features"
|
modulo the nit where i'd recommend remove "CEPH_FEATURES_SUPPORTED_DEFAULT != features", lgtm. |
… features This ensures we reencode the payload with the appropriate set of features if the client, us, or the target do not have identical features. Otherwise we may forward an encoding with more features than the target can handle. Signed-off-by: Sage Weil <sage@redhat.com>
Normally we never call encode on a message that has a byte_throttler set because we only use it for messages we received. However, for forwarded messages that we clear_payload() before resending, we *do* reencode, and in that case we need to retake the appropriate number of bytes from the throttler--just like we release them in clear_payload(). Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
092a3af to
e7bf50b
Compare
This ensures we reencode the payload with the
appropriate set of features if the client, us, or the
target do not have identical features. Otherwise we
may forward an encoding with more features than the
target can handle.
Signed-off-by: Sage Weil sage@redhat.com