From 2b2f05fea3df031941f1bb7f2bd6728fc0775282 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Wed, 15 Aug 2018 20:31:36 +0800 Subject: [PATCH] auth: switch to LockPolicy templatized alternatives this change * is the first step to use std::mutex, etc directly instead our Mutex wrappers for better performance. see https://trello.com/c/aUSezBRH/365-stdmutex-etc-for-release-builds * pave the road to lockless auth/ subsystem in seastar. Signed-off-by: Kefu Chai --- src/auth/AuthAuthorizeHandler.h | 1 - src/auth/AuthClientHandler.cc | 12 ++++++--- src/auth/AuthClientHandler.h | 17 +++++++----- src/auth/RotatingKeyRing.cc | 35 +++++++++++++++--------- src/auth/RotatingKeyRing.h | 7 ++--- src/auth/cephx/CephxClientHandler.cc | 39 ++++++++++++++++----------- src/auth/cephx/CephxClientHandler.h | 21 ++++++++++----- src/auth/none/AuthNoneClientHandler.h | 18 ++++++++----- src/common/shared_cache.hpp | 2 +- src/mds/MDSDaemon.cc | 3 +-- src/mgr/DaemonServer.cc | 3 +-- src/mon/MonClient.cc | 9 ++++--- src/mon/MonClient.h | 16 +++++------ src/mon/Monitor.cc | 2 +- src/osd/OSD.cc | 2 +- 15 files changed, 113 insertions(+), 74 deletions(-) diff --git a/src/auth/AuthAuthorizeHandler.h b/src/auth/AuthAuthorizeHandler.h index f6a0e68c959..7473267dc4d 100644 --- a/src/auth/AuthAuthorizeHandler.h +++ b/src/auth/AuthAuthorizeHandler.h @@ -27,7 +27,6 @@ class CephContext; class KeyRing; -class RotatingKeyRing; struct AuthAuthorizeHandler { virtual ~AuthAuthorizeHandler() {} diff --git a/src/auth/AuthClientHandler.cc b/src/auth/AuthClientHandler.cc index 8a75a304289..320d2faba4e 100644 --- a/src/auth/AuthClientHandler.cc +++ b/src/auth/AuthClientHandler.cc @@ -20,16 +20,20 @@ #include "none/AuthNoneClientHandler.h" -AuthClientHandler* AuthClientHandler::create(CephContext *cct, int proto, - RotatingKeyRing *rkeys) +template +AuthClientHandler* +AuthClientHandler::create(CephContext *cct, int proto, + RotatingKeyRing *rkeys) { switch (proto) { case CEPH_AUTH_CEPHX: - return new CephxClientHandler(cct, rkeys); + return new CephxClientHandler(cct, rkeys); case CEPH_AUTH_NONE: - return new AuthNoneClientHandler(cct, rkeys); + return new AuthNoneClientHandler(cct, rkeys); default: return NULL; } } +// explicitly instantiate only the classes we need +template class AuthClientHandler; diff --git a/src/auth/AuthClientHandler.h b/src/auth/AuthClientHandler.h index be1ad642bad..140dbcc63b4 100644 --- a/src/auth/AuthClientHandler.h +++ b/src/auth/AuthClientHandler.h @@ -17,12 +17,14 @@ #include "auth/Auth.h" -#include "common/RWLock.h" +#include "common/lock_policy.h" +#include "common/lock_shared_mutex.h" class CephContext; struct MAuthReply; -class RotatingKeyRing; +template class RotatingKeyRing; +template class AuthClientHandler { protected: CephContext *cct; @@ -31,18 +33,19 @@ protected: uint32_t want; uint32_t have; uint32_t need; - RWLock lock; + mutable SharedMutexT lock; public: explicit AuthClientHandler(CephContext *cct_) : cct(cct_), global_id(0), want(CEPH_ENTITY_TYPE_AUTH), have(0), need(0), - lock("AuthClientHandler::lock") {} + lock{SharedMutex::create("AuthClientHandler::lock")} + {} virtual ~AuthClientHandler() {} void init(const EntityName& n) { name = n; } void set_want_keys(__u32 keys) { - RWLock::WLocker l(lock); + std::unique_lock l{lock}; want = keys | CEPH_ENTITY_TYPE_AUTH; validate_tickets(); } @@ -61,8 +64,8 @@ public: virtual void set_global_id(uint64_t id) = 0; - static AuthClientHandler* create(CephContext *cct, - int proto, RotatingKeyRing *rkeys); + static AuthClientHandler* + create(CephContext *cct, int proto, RotatingKeyRing *rkeys); protected: virtual void validate_tickets() = 0; }; diff --git a/src/auth/RotatingKeyRing.cc b/src/auth/RotatingKeyRing.cc index e48127aa64b..196b1a12052 100644 --- a/src/auth/RotatingKeyRing.cc +++ b/src/auth/RotatingKeyRing.cc @@ -9,26 +9,30 @@ #define dout_prefix *_dout << "auth: " -bool RotatingKeyRing::need_new_secrets() const +template +bool RotatingKeyRing::need_new_secrets() const { - Mutex::Locker l(lock); + std::lock_guard l{lock}; return secrets.need_new_secrets(); } -bool RotatingKeyRing::need_new_secrets(utime_t now) const +template +bool RotatingKeyRing::need_new_secrets(utime_t now) const { - Mutex::Locker l(lock); + std::lock_guard l{lock}; return secrets.need_new_secrets(now); } -void RotatingKeyRing::set_secrets(RotatingSecrets&& s) +template +void RotatingKeyRing::set_secrets(RotatingSecrets&& s) { - Mutex::Locker l(lock); + std::lock_guard l{lock}; secrets = std::move(s); dump_rotating(); } -void RotatingKeyRing::dump_rotating() const +template +void RotatingKeyRing::dump_rotating() const { ldout(cct, 10) << "dump_rotating:" << dendl; for (map::const_iterator iter = secrets.secrets.begin(); @@ -37,16 +41,18 @@ void RotatingKeyRing::dump_rotating() const ldout(cct, 10) << " id " << iter->first << " " << iter->second << dendl; } -bool RotatingKeyRing::get_secret(const EntityName& name, CryptoKey& secret) const +template +bool RotatingKeyRing::get_secret(const EntityName& name, CryptoKey& secret) const { - Mutex::Locker l(lock); + std::lock_guard l{lock}; return keyring->get_secret(name, secret); } -bool RotatingKeyRing::get_service_secret(uint32_t service_id_, uint64_t secret_id, +template +bool RotatingKeyRing::get_service_secret(uint32_t service_id_, uint64_t secret_id, CryptoKey& secret) const { - Mutex::Locker l(lock); + std::lock_guard l{lock}; if (service_id_ != this->service_id) { ldout(cct, 0) << "do not have service " << ceph_entity_type_name(service_id_) @@ -66,8 +72,11 @@ bool RotatingKeyRing::get_service_secret(uint32_t service_id_, uint64_t secret_i return true; } -KeyRing *RotatingKeyRing:: -get_keyring() +template +KeyRing *RotatingKeyRing::get_keyring() { return keyring; } + +// explicitly instantiate only the classes we need +template class RotatingKeyRing; diff --git a/src/auth/RotatingKeyRing.h b/src/auth/RotatingKeyRing.h index 729f5c52021..6d094c22ef7 100644 --- a/src/auth/RotatingKeyRing.h +++ b/src/auth/RotatingKeyRing.h @@ -15,7 +15,7 @@ #ifndef CEPH_ROTATINGKEYRING_H #define CEPH_ROTATINGKEYRING_H -#include "common/Mutex.h" +#include "common/lock_mutex.h" #include "auth/Auth.h" /* @@ -25,19 +25,20 @@ class KeyRing; class CephContext; +template class RotatingKeyRing : public KeyStore { CephContext *cct; uint32_t service_id; RotatingSecrets secrets; KeyRing *keyring; - mutable Mutex lock; + mutable LockMutexT lock; public: RotatingKeyRing(CephContext *cct_, uint32_t s, KeyRing *kr) : cct(cct_), service_id(s), keyring(kr), - lock("RotatingKeyRing::lock") {} + lock(LockMutex::create("RotatingKeyRing::lock")) {} bool need_new_secrets() const; bool need_new_secrets(utime_t now) const; diff --git a/src/auth/cephx/CephxClientHandler.cc b/src/auth/cephx/CephxClientHandler.cc index 88eec0b2a41..ea0b67cebbb 100644 --- a/src/auth/cephx/CephxClientHandler.cc +++ b/src/auth/cephx/CephxClientHandler.cc @@ -27,12 +27,12 @@ #undef dout_prefix #define dout_prefix *_dout << "cephx client: " - -int CephxClientHandler::build_request(bufferlist& bl) const +template +int CephxClientHandler::build_request(bufferlist& bl) const { ldout(cct, 10) << "build_request" << dendl; - RWLock::RLocker l(lock); + std::shared_lock l{lock}; if (need & CEPH_ENTITY_TYPE_AUTH) { /* authenticate */ @@ -98,7 +98,8 @@ int CephxClientHandler::build_request(bufferlist& bl) const return 0; } -bool CephxClientHandler::_need_tickets() const +template +bool CephxClientHandler::_need_tickets() const { // do not bother (re)requesting tickets if we *only* need the MGR // ticket; that can happen during an upgrade and we want to avoid a @@ -107,10 +108,11 @@ bool CephxClientHandler::_need_tickets() const return need && need != CEPH_ENTITY_TYPE_MGR; } -int CephxClientHandler::handle_response(int ret, bufferlist::const_iterator& indata) +template +int CephxClientHandler::handle_response(int ret, bufferlist::const_iterator& indata) { ldout(cct, 10) << "handle_response ret = " << ret << dendl; - RWLock::WLocker l(lock); + std::unique_lock l{lock}; if (ret < 0) return ret; // hrm! @@ -201,16 +203,17 @@ int CephxClientHandler::handle_response(int ret, bufferlist::const_iterator& ind } - -AuthAuthorizer *CephxClientHandler::build_authorizer(uint32_t service_id) const +template +AuthAuthorizer *CephxClientHandler::build_authorizer(uint32_t service_id) const { - RWLock::RLocker l(lock); + std::shared_lock l{lock}; ldout(cct, 10) << "build_authorizer for service " << ceph_entity_type_name(service_id) << dendl; return tickets.build_authorizer(service_id); } -bool CephxClientHandler::build_rotating_request(bufferlist& bl) const +template +bool CephxClientHandler::build_rotating_request(bufferlist& bl) const { ldout(cct, 10) << "build_rotating_request" << dendl; CephXRequestHeader header; @@ -219,9 +222,10 @@ bool CephxClientHandler::build_rotating_request(bufferlist& bl) const return true; } -void CephxClientHandler::prepare_build_request() +template +void CephxClientHandler::prepare_build_request() { - RWLock::WLocker l(lock); + std::unique_lock l{lock}; ldout(cct, 10) << "validate_tickets: want=" << want << " need=" << need << " have=" << have << dendl; validate_tickets(); @@ -231,15 +235,17 @@ void CephxClientHandler::prepare_build_request() ticket_handler = &(tickets.get_handler(CEPH_ENTITY_TYPE_AUTH)); } -void CephxClientHandler::validate_tickets() +template +void CephxClientHandler::validate_tickets() { // lock should be held for write tickets.validate_tickets(want, have, need); } -bool CephxClientHandler::need_tickets() +template +bool CephxClientHandler::need_tickets() { - RWLock::WLocker l(lock); + std::unique_lock l{lock}; validate_tickets(); ldout(cct, 20) << "need_tickets: want=" << want @@ -249,3 +255,6 @@ bool CephxClientHandler::need_tickets() return _need_tickets(); } + +// explicitly instantiate only the classes we need +template class CephxClientHandler; diff --git a/src/auth/cephx/CephxClientHandler.h b/src/auth/cephx/CephxClientHandler.h index 5d7b489b204..eb6055c739e 100644 --- a/src/auth/cephx/CephxClientHandler.h +++ b/src/auth/cephx/CephxClientHandler.h @@ -22,7 +22,8 @@ class CephContext; class KeyRing; -class CephxClientHandler : public AuthClientHandler { +template +class CephxClientHandler : public AuthClientHandler { bool starting; /* envelope protocol parameters */ @@ -31,12 +32,20 @@ class CephxClientHandler : public AuthClientHandler { CephXTicketManager tickets; CephXTicketHandler* ticket_handler; - RotatingKeyRing *rotating_secrets; + RotatingKeyRing *rotating_secrets; KeyRing *keyring; + using AuthClientHandler::cct; + using AuthClientHandler::global_id; + using AuthClientHandler::want; + using AuthClientHandler::have; + using AuthClientHandler::need; + using AuthClientHandler::lock; + public: - CephxClientHandler(CephContext *cct_, RotatingKeyRing *rsecrets) - : AuthClientHandler(cct_), + CephxClientHandler(CephContext *cct_, + RotatingKeyRing *rsecrets) + : AuthClientHandler(cct_), starting(false), server_challenge(0), tickets(cct_), @@ -48,7 +57,7 @@ public: } void reset() override { - RWLock::WLocker l(lock); + std::unique_lock l{lock}; starting = true; server_challenge = 0; } @@ -64,7 +73,7 @@ public: bool need_tickets() override; void set_global_id(uint64_t id) override { - RWLock::WLocker l(lock); + std::unique_lock l{lock}; global_id = id; tickets.global_id = id; } diff --git a/src/auth/none/AuthNoneClientHandler.h b/src/auth/none/AuthNoneClientHandler.h index ba843979e09..32a410ab76d 100644 --- a/src/auth/none/AuthNoneClientHandler.h +++ b/src/auth/none/AuthNoneClientHandler.h @@ -19,11 +19,17 @@ #include "AuthNoneProtocol.h" #include "common/ceph_context.h" #include "common/config.h" - -class AuthNoneClientHandler : public AuthClientHandler { + +template +class AuthNoneClientHandler : public AuthClientHandler { + using AuthClientHandler::cct; + using AuthClientHandler::global_id; + using AuthClientHandler::lock; + public: - AuthNoneClientHandler(CephContext *cct_, RotatingKeyRing *rkeys) - : AuthClientHandler(cct_) {} + AuthNoneClientHandler(CephContext *cct_, + RotatingKeyRing *rkeys) + : AuthClientHandler(cct_) {} void reset() override { } @@ -35,7 +41,7 @@ public: int get_protocol() const override { return CEPH_AUTH_NONE; } AuthAuthorizer *build_authorizer(uint32_t service_id) const override { - RWLock::RLocker l(lock); + std::shared_lock l{lock}; AuthNoneAuthorizer *auth = new AuthNoneAuthorizer(); if (auth) { auth->build_authorizer(cct->_conf->name, global_id); @@ -46,7 +52,7 @@ public: bool need_tickets() override { return false; } void set_global_id(uint64_t id) override { - RWLock::WLocker l(lock); + std::unique_lock l{lock}; global_id = id; } private: diff --git a/src/common/shared_cache.hpp b/src/common/shared_cache.hpp index 2fdfff58136..1375e4e57a9 100644 --- a/src/common/shared_cache.hpp +++ b/src/common/shared_cache.hpp @@ -35,7 +35,7 @@ class SharedLRU { using WeakVPtr = typename shared_ptr_trait_t::template weak_ptr; LockMutexT lock; size_t max_size; - LockCond cond; + LockCondT cond; unsigned size; public: int waiting; diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc index 98c524556d7..a64dad905b2 100644 --- a/src/mds/MDSDaemon.cc +++ b/src/mds/MDSDaemon.cc @@ -1323,8 +1323,7 @@ bool MDSDaemon::ms_verify_authorizer(Connection *con, int peer_type, EntityName name; uint64_t global_id; - RotatingKeyRing *keys = monc->rotating_secrets.get(); - if (keys) { + if (auto keys = monc->rotating_secrets.get(); keys) { is_valid = authorize_handler->verify_authorizer( cct, keys, authorizer_data, authorizer_reply, name, global_id, caps_info, diff --git a/src/mgr/DaemonServer.cc b/src/mgr/DaemonServer.cc index ebdacaf6901..702bf1629b5 100644 --- a/src/mgr/DaemonServer.cc +++ b/src/mgr/DaemonServer.cc @@ -188,8 +188,7 @@ bool DaemonServer::ms_verify_authorizer( s->inst.addr = con->get_peer_addr(); AuthCapsInfo caps_info; - RotatingKeyRing *keys = monc->rotating_secrets.get(); - if (keys) { + if (auto keys = monc->rotating_secrets.get(); keys) { is_valid = handler->verify_authorizer( cct, keys, authorizer_data, diff --git a/src/mon/MonClient.cc b/src/mon/MonClient.cc index bbbf0ab99e4..0907c2a8ea0 100644 --- a/src/mon/MonClient.cc +++ b/src/mon/MonClient.cc @@ -427,7 +427,7 @@ int MonClient::init() } rotating_secrets.reset( - new RotatingKeyRing(cct, cct->get_module_type(), keyring.get())); + new RotatingKeyRing(cct, cct->get_module_type(), keyring.get())); initialized = true; @@ -1250,7 +1250,7 @@ void MonConnection::start(epoch_t epoch, int MonConnection::handle_auth(MAuthReply* m, const EntityName& entity_name, uint32_t want_keys, - RotatingKeyRing* keyring) + RotatingKeyRing* keyring) { if (state == State::NEGOTIATING) { int r = _negotiate(m, entity_name, want_keys, keyring); @@ -1269,7 +1269,7 @@ int MonConnection::handle_auth(MAuthReply* m, int MonConnection::_negotiate(MAuthReply *m, const EntityName& entity_name, uint32_t want_keys, - RotatingKeyRing* keyring) + RotatingKeyRing* keyring) { if (auth && (int)m->protocol == auth->get_protocol()) { // good, negotiation completed @@ -1277,7 +1277,8 @@ int MonConnection::_negotiate(MAuthReply *m, return 0; } - auth.reset(AuthClientHandler::create(cct, m->protocol, keyring)); + auth.reset( + AuthClientHandler::create(cct,m->protocol, keyring)); if (!auth) { ldout(cct, 10) << "no handler for protocol " << m->protocol << dendl; if (m->result == -ENOTSUP) { diff --git a/src/mon/MonClient.h b/src/mon/MonClient.h index 2c0f2180593..bffd2d916e6 100644 --- a/src/mon/MonClient.h +++ b/src/mon/MonClient.h @@ -34,9 +34,9 @@ class MAuthRotating; class LogClient; class AuthAuthorizer; class AuthMethodList; -class AuthClientHandler; +template class AuthClientHandler; class KeyRing; -class RotatingKeyRing; +template class RotatingKeyRing; struct MonClientPinger : public Dispatcher { @@ -106,7 +106,7 @@ public: int handle_auth(MAuthReply *m, const EntityName& entity_name, uint32_t want_keys, - RotatingKeyRing* keyring); + RotatingKeyRing* keyring); int authenticate(MAuthReply *m); void start(epoch_t epoch, const EntityName& entity_name, @@ -118,7 +118,7 @@ public: ConnectionRef get_con() { return con; } - std::unique_ptr& get_auth() { + std::unique_ptr>& get_auth() { return auth; } @@ -126,7 +126,7 @@ private: int _negotiate(MAuthReply *m, const EntityName& entity_name, uint32_t want_keys, - RotatingKeyRing* keyring); + RotatingKeyRing* keyring); private: CephContext *cct; @@ -139,7 +139,7 @@ private: State state = State::NONE; ConnectionRef con; - std::unique_ptr auth; + std::unique_ptr> auth; uint64_t global_id; }; @@ -192,7 +192,7 @@ private: bool got_config = false; // authenticate - std::unique_ptr auth; + std::unique_ptr> auth; uint32_t want_keys = 0; uint64_t global_id = 0; Cond auth_cond; @@ -337,7 +337,7 @@ public: } std::unique_ptr keyring; - std::unique_ptr rotating_secrets; + std::unique_ptr> rotating_secrets; public: explicit MonClient(CephContext *cct_); diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index bf93a756602..8da36176c0a 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -5713,7 +5713,7 @@ bool Monitor::ms_get_authorizer(int service_id, AuthAuthorizer **authorizer, if (!auth_cluster_required.is_supported_auth(CEPH_AUTH_CEPHX)) { // auth_none dout(20) << __func__ << " building auth_none authorizer" << dendl; - AuthNoneClientHandler handler(g_ceph_context, nullptr); + AuthNoneClientHandler handler(g_ceph_context, nullptr); handler.set_global_id(0); *authorizer = handler.build_authorizer(service_id); return true; diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc index e83e29b8bb6..26da1801d20 100644 --- a/src/osd/OSD.cc +++ b/src/osd/OSD.cc @@ -6618,7 +6618,7 @@ bool OSD::ms_verify_authorizer( uint64_t global_id; uint64_t auid = CEPH_AUTH_UID_DEFAULT; - RotatingKeyRing *keys = monc->rotating_secrets.get(); + auto keys = monc->rotating_secrets.get(); if (keys) { isvalid = authorize_handler->verify_authorizer( cct, keys, -- 2.39.5