From 507d213cc453ed86ab38619590f710f33245c652 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sat, 8 Feb 2020 18:44:51 -0600 Subject: [PATCH] msg/Policy: limit unregistered anon connections to mon When we were fixing 'tell mon' we changed the messenger to allow multiple lossy connections to the same server. This was needed for the mon, and assumed to be harmless for everyone else. However, for the OSD, it can lead to out-of-order requests, as observed in this bug: https://tracker.ceph.com/issues/42328 Fix by reverting this behavior change except for the mon: - Introduce a stateless_anon_server policy that sets register_lossy_clients = false - Effectively revert the behavior change in c48a29b9edde3c6d3cd34252d202885e2e064fe0 so that lossy clients *are* registered for stateless_server policy. - Use the state_anon_server policy for the mon, which was the one place where we needed unregistered connections (for 'tell mon.x' to work). Fixes: https://tracker.ceph.com/issues/42328 Signed-off-by: Sage Weil --- src/ceph_mon.cc | 8 ++++---- src/msg/Policy.h | 26 +++++++++++++++++++------- src/msg/async/ProtocolV1.cc | 3 ++- src/msg/async/ProtocolV2.cc | 3 ++- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/ceph_mon.cc b/src/ceph_mon.cc index f46607ed0b25a..117a94dc55ccb 100644 --- a/src/ceph_mon.cc +++ b/src/ceph_mon.cc @@ -765,17 +765,17 @@ int main(int argc, const char **argv) msgr->set_cluster_protocol(CEPH_MON_PROTOCOL); msgr->set_default_send_priority(CEPH_MSG_PRIO_HIGH); - msgr->set_default_policy(Messenger::Policy::stateless_server(0)); + msgr->set_default_policy(Messenger::Policy::stateless_anon_server(0)); msgr->set_policy(entity_name_t::TYPE_MON, Messenger::Policy::lossless_peer_reuse( CEPH_FEATURE_SERVER_LUMINOUS)); msgr->set_policy(entity_name_t::TYPE_OSD, - Messenger::Policy::stateless_server( + Messenger::Policy::stateless_anon_server( CEPH_FEATURE_SERVER_LUMINOUS)); msgr->set_policy(entity_name_t::TYPE_CLIENT, - Messenger::Policy::stateless_server(0)); + Messenger::Policy::stateless_anon_server(0)); msgr->set_policy(entity_name_t::TYPE_MDS, - Messenger::Policy::stateless_server(0)); + Messenger::Policy::stateless_anon_server(0)); // throttle client traffic Throttle *client_throttler = new Throttle(g_ceph_context, "mon_client_bytes", diff --git a/src/msg/Policy.h b/src/msg/Policy.h index 5d13ffb868aa2..9f5ea32a0bd95 100644 --- a/src/msg/Policy.h +++ b/src/msg/Policy.h @@ -25,6 +25,14 @@ struct Policy { bool standby; /// If true, we will try to detect session resets bool resetcheck; + + /// Server: register lossy client connections. + bool register_lossy_clients = true; + // The net result of this is that a given client can only have one + // open connection with the server. If a new connection is made, + // the old (registered) one is closed by the messenger during the accept + // process. + /** * The throttler is used to limit how much data is held by Messages from * the associated Connection(s). When reading in a new Message, the Messenger @@ -45,8 +53,9 @@ struct Policy { features_supported(CEPH_FEATURES_SUPPORTED_DEFAULT), features_required(0) {} private: - Policy(bool l, bool s, bool st, bool r, uint64_t req) + Policy(bool l, bool s, bool st, bool r, bool rlc, uint64_t req) : lossy(l), server(s), standby(st), resetcheck(r), + register_lossy_clients(rlc), throttler_bytes(NULL), throttler_messages(NULL), features_supported(CEPH_FEATURES_SUPPORTED_DEFAULT), @@ -54,22 +63,25 @@ private: public: static Policy stateful_server(uint64_t req) { - return Policy(false, true, true, true, req); + return Policy(false, true, true, true, true, req); } static Policy stateless_server(uint64_t req) { - return Policy(true, true, false, false, req); + return Policy(true, true, false, false, true, req); + } + static Policy stateless_anon_server(uint64_t req) { + return Policy(true, true, false, false, false, req); } static Policy lossless_peer(uint64_t req) { - return Policy(false, false, true, false, req); + return Policy(false, false, true, false, true, req); } static Policy lossless_peer_reuse(uint64_t req) { - return Policy(false, false, true, true, req); + return Policy(false, false, true, true, true, req); } static Policy lossy_client(uint64_t req) { - return Policy(true, false, false, false, req); + return Policy(true, false, false, false, true, req); } static Policy lossless_client(uint64_t req) { - return Policy(false, false, false, true, req); + return Policy(false, false, false, true, true, req); } }; diff --git a/src/msg/async/ProtocolV1.cc b/src/msg/async/ProtocolV1.cc index 4e890a195c319..c0620da1d4528 100644 --- a/src/msg/async/ProtocolV1.cc +++ b/src/msg/async/ProtocolV1.cc @@ -2062,7 +2062,8 @@ CtPtr ProtocolV1::handle_connect_message_2() { ldout(cct, 10) << __func__ << " accept setting up session_security." << dendl; if (connection->policy.server && - connection->policy.lossy) { + connection->policy.lossy && + !connection->policy.register_lossy_clients) { // incoming lossy client, no need to register this connection // new session ldout(cct, 10) << __func__ << " accept new session" << dendl; diff --git a/src/msg/async/ProtocolV2.cc b/src/msg/async/ProtocolV2.cc index fe2158289a50b..921c8d516623e 100644 --- a/src/msg/async/ProtocolV2.cc +++ b/src/msg/async/ProtocolV2.cc @@ -2369,7 +2369,8 @@ CtPtr ProtocolV2::handle_client_ident(ceph::bufferlist &payload) peer_global_seq = client_ident.global_seq(); if (connection->policy.server && - connection->policy.lossy) { + connection->policy.lossy && + connection->policy.register_lossy_clients) { // incoming lossy client, no need to register this connection } else { // Looks good so far, let's check if there is already an existing connection -- 2.47.3