From 2a765ea220106faac2714b9514e9ca0118b6758a Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Mon, 8 Jul 2019 10:30:26 -0700 Subject: [PATCH] mds: apply configuration changes through MDSRank This avoids the need to acquire the mds_lock prior to responding to config changes. The MDSRank can create a finisher context that acquires the lock later. Fixes: https://tracker.ceph.com/issues/40694 Signed-off-by: Patrick Donnelly --- src/mds/MDBalancer.cc | 4 +- src/mds/MDBalancer.h | 4 +- src/mds/MDCache.cc | 8 ++- src/mds/MDCache.h | 4 +- src/mds/MDSDaemon.cc | 113 ------------------------------------------ src/mds/MDSDaemon.h | 7 +-- src/mds/MDSRank.cc | 88 +++++++++++++++++++++++++++++++- src/mds/MDSRank.h | 15 ++---- src/mds/Migrator.cc | 6 +-- src/mds/Migrator.h | 4 +- src/mds/PurgeQueue.cc | 6 +-- src/mds/PurgeQueue.h | 4 +- src/mds/Server.cc | 3 +- src/mds/Server.h | 3 +- src/mds/SessionMap.cc | 3 +- src/mds/SessionMap.h | 3 +- 16 files changed, 108 insertions(+), 167 deletions(-) diff --git a/src/mds/MDBalancer.cc b/src/mds/MDBalancer.cc index 2cb109a4e1185..375866460cc73 100644 --- a/src/mds/MDBalancer.cc +++ b/src/mds/MDBalancer.cc @@ -82,9 +82,7 @@ MDBalancer::MDBalancer(MDSRank *m, Messenger *msgr, MonClient *monc) : bal_fragment_interval = g_conf().get_val("mds_bal_fragment_interval"); } -void MDBalancer::handle_conf_change(const ConfigProxy& conf, - const std::set &changed, - const MDSMap &mds_map) +void MDBalancer::handle_conf_change(const std::set& changed, const MDSMap& mds_map) { if (changed.count("mds_bal_fragment_dirs")) bal_fragment_dirs = g_conf().get_val("mds_bal_fragment_dirs"); diff --git a/src/mds/MDBalancer.h b/src/mds/MDBalancer.h index ce48c6febaaab..18661f7bed768 100644 --- a/src/mds/MDBalancer.h +++ b/src/mds/MDBalancer.h @@ -44,9 +44,7 @@ public: MDBalancer(MDSRank *m, Messenger *msgr, MonClient *monc); - void handle_conf_change(const ConfigProxy& conf, - const std::set &changed, - const MDSMap &mds_map); + void handle_conf_change(const std::set& changed, const MDSMap& mds_map); int proc_message(const cref_t &m); diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc index feaa71289a1be..ca3cfb4b2f49f 100644 --- a/src/mds/MDCache.cc +++ b/src/mds/MDCache.cc @@ -198,9 +198,7 @@ MDCache::~MDCache() } } -void MDCache::handle_conf_change(const ConfigProxy& conf, - const std::set &changed, - const MDSMap &mdsmap) +void MDCache::handle_conf_change(const std::set& changed, const MDSMap& mdsmap) { if (changed.count("mds_cache_size")) cache_inode_limit = g_conf().get_val("mds_cache_size"); @@ -216,8 +214,8 @@ void MDCache::handle_conf_change(const ConfigProxy& conf, trim_counter = DecayCounter(g_conf().get_val("mds_cache_trim_decay_rate")); } - migrator->handle_conf_change(conf, changed, mdsmap); - mds->balancer->handle_conf_change(conf, changed, mdsmap); + migrator->handle_conf_change(changed, mdsmap); + mds->balancer->handle_conf_change(changed, mdsmap); } void MDCache::log_stat() diff --git a/src/mds/MDCache.h b/src/mds/MDCache.h index 9898fc37c0667..d6340f0ad5f53 100644 --- a/src/mds/MDCache.h +++ b/src/mds/MDCache.h @@ -774,9 +774,7 @@ public: public: explicit MDCache(MDSRank *m, PurgeQueue &purge_queue_); ~MDCache(); - void handle_conf_change(const ConfigProxy& conf, - const std::set &changed, - const MDSMap &mds_map); + void handle_conf_change(const std::set& changed, const MDSMap& mds_map); // debug void log_stat(); diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc index 6d3cad07b859d..cb410a4b45b88 100644 --- a/src/mds/MDSDaemon.cc +++ b/src/mds/MDSDaemon.cc @@ -337,110 +337,6 @@ void MDSDaemon::clean_up_admin_socket() asok_hook = NULL; } -const char** MDSDaemon::get_tracked_conf_keys() const -{ - static const char* KEYS[] = { - "mds_op_complaint_time", "mds_op_log_threshold", - "mds_op_history_size", "mds_op_history_duration", - "mds_enable_op_tracker", - "mds_log_pause", - // clog & admin clog - "clog_to_monitors", - "clog_to_syslog", - "clog_to_syslog_facility", - "clog_to_syslog_level", - "clog_to_graylog", - "clog_to_graylog_host", - "clog_to_graylog_port", - // MDCache - "mds_cache_size", - "mds_cache_memory_limit", - "mds_cache_reservation", - "mds_health_cache_threshold", - "mds_cache_mid", - "mds_dump_cache_threshold_formatter", - "mds_cache_trim_decay_rate", - "mds_dump_cache_threshold_file", - // MDBalancer - "mds_bal_fragment_dirs", - "mds_bal_fragment_interval", - // PurgeQueue - "mds_max_purge_ops", - "mds_max_purge_ops_per_pg", - "mds_max_purge_files", - // Migrator - "mds_max_export_size", - "mds_inject_migrator_session_race", - "host", - "fsid", - "mds_cap_revoke_eviction_timeout", - // SessionMap - "mds_request_load_average_decay_rate", - "mds_recall_max_decay_rate", - NULL - }; - return KEYS; -} - -void MDSDaemon::handle_conf_change(const ConfigProxy& conf, - const std::set &changed) -{ - // We may be called within mds_lock (via `tell`) or outwith the - // lock (via admin socket `config set`), so handle either case. - const bool initially_locked = mds_lock.is_locked_by_me(); - if (!initially_locked) { - mds_lock.Lock(); - } - - if (changed.count("mds_op_complaint_time") || - changed.count("mds_op_log_threshold")) { - if (mds_rank) { - mds_rank->op_tracker.set_complaint_and_threshold(conf->mds_op_complaint_time, - conf->mds_op_log_threshold); - } - } - if (changed.count("mds_op_history_size") || - changed.count("mds_op_history_duration")) { - if (mds_rank) { - mds_rank->op_tracker.set_history_size_and_duration(conf->mds_op_history_size, - conf->mds_op_history_duration); - } - } - if (changed.count("mds_enable_op_tracker")) { - if (mds_rank) { - mds_rank->op_tracker.set_tracking(conf->mds_enable_op_tracker); - } - } - if (changed.count("clog_to_monitors") || - changed.count("clog_to_syslog") || - changed.count("clog_to_syslog_level") || - changed.count("clog_to_syslog_facility") || - changed.count("clog_to_graylog") || - changed.count("clog_to_graylog_host") || - changed.count("clog_to_graylog_port") || - changed.count("host") || - changed.count("fsid")) { - if (mds_rank) { - mds_rank->update_log_config(); - } - } - - if (!g_conf()->mds_log_pause && changed.count("mds_log_pause")) { - if (mds_rank) { - mds_rank->mdlog->kick_submitter(); - } - } - - if (mds_rank) { - mds_rank->handle_conf_change(conf, changed); - } - - if (!initially_locked) { - mds_lock.Unlock(); - } -} - - int MDSDaemon::init() { dout(10) << sizeof(MDSCacheObject) << "\tMDSCacheObject" << dendl; @@ -532,7 +428,6 @@ int MDSDaemon::init() // Set up admin socket before taking mds_lock, so that ordering // is consistent (later we take mds_lock within asok callbacks) set_up_admin_socket(); - g_conf().add_observer(this); mds_lock.Lock(); if (beacon.get_want_state() == MDSMap::STATE_DNE) { suicide(); // we could do something more graceful here @@ -1042,14 +937,6 @@ void MDSDaemon::suicide() tick_event = 0; } - //because add_observer is called after set_up_admin_socket - //so we can use asok_hook to avoid assert in the remove_observer - if (asok_hook != NULL) { - mds_lock.Unlock(); - g_conf().remove_observer(this); - mds_lock.Lock(); - } - clean_up_admin_socket(); // Inform MDS we are going away, then shut down beacon diff --git a/src/mds/MDSDaemon.h b/src/mds/MDSDaemon.h index 4603f0512ee75..13b29d19d6b59 100644 --- a/src/mds/MDSDaemon.h +++ b/src/mds/MDSDaemon.h @@ -40,7 +40,7 @@ class Messenger; class MonClient; -class MDSDaemon : public Dispatcher, public md_config_obs_t { +class MDSDaemon : public Dispatcher { public: /* Global MDS lock: every time someone takes this, they must * also check the `stopping` flag. If stopping is true, you @@ -92,11 +92,6 @@ class MDSDaemon : public Dispatcher, public md_config_obs_t { * such that deleting xlists doesn't assert. */ bool is_clean_shutdown(); - - // config observer bits - const char** get_tracked_conf_keys() const override; - void handle_conf_change(const ConfigProxy& conf, - const std::set &changed) override; protected: // tick and other timer fun Context *tick_event = nullptr; diff --git a/src/mds/MDSRank.cc b/src/mds/MDSRank.cc index f683a7bdd1179..76fe4ce3d88a3 100644 --- a/src/mds/MDSRank.cc +++ b/src/mds/MDSRank.cc @@ -825,6 +825,8 @@ void MDSRankDispatcher::shutdown() dout(1) << __func__ << ": shutting down rank " << whoami << dendl; + g_conf().remove_observer(this); + timer.shutdown(); // MDLog has to shut down before the finisher, because some of its @@ -3471,7 +3473,9 @@ MDSRankDispatcher::MDSRankDispatcher( Context *suicide_hook_) : MDSRank(whoami_, mds_lock_, clog_, timer_, beacon_, mdsmap_, msgr, monc_, respawn_hook_, suicide_hook_) -{} +{ + g_conf().add_observer(this); +} bool MDSRankDispatcher::handle_command( const cmdmap_t &cmdmap, @@ -3595,3 +3599,85 @@ epoch_t MDSRank::get_osd_epoch() const return objecter->with_osdmap(std::mem_fn(&OSDMap::get_epoch)); } +const char** MDSRankDispatcher::get_tracked_conf_keys() const +{ + static const char* KEYS[] = { + "mds_op_complaint_time", "mds_op_log_threshold", + "mds_op_history_size", "mds_op_history_duration", + "mds_enable_op_tracker", + "mds_log_pause", + // clog & admin clog + "clog_to_monitors", + "clog_to_syslog", + "clog_to_syslog_facility", + "clog_to_syslog_level", + "clog_to_graylog", + "clog_to_graylog_host", + "clog_to_graylog_port", + // MDCache + "mds_cache_size", + "mds_cache_memory_limit", + "mds_cache_reservation", + "mds_health_cache_threshold", + "mds_cache_mid", + "mds_dump_cache_threshold_formatter", + "mds_cache_trim_decay_rate", + "mds_dump_cache_threshold_file", + // MDBalancer + "mds_bal_fragment_dirs", + "mds_bal_fragment_interval", + // PurgeQueue + "mds_max_purge_ops", + "mds_max_purge_ops_per_pg", + "mds_max_purge_files", + // Migrator + "mds_max_export_size", + "mds_inject_migrator_session_race", + "host", + "fsid", + "mds_cap_revoke_eviction_timeout", + // SessionMap + "mds_request_load_average_decay_rate", + "mds_recall_max_decay_rate", + NULL + }; + return KEYS; +} + +void MDSRankDispatcher::handle_conf_change(const ConfigProxy& conf, const std::set& changed) +{ + // XXX with or without mds_lock! + + if (changed.count("mds_op_complaint_time") || changed.count("mds_op_log_threshold")) { + op_tracker.set_complaint_and_threshold(conf->mds_op_complaint_time, conf->mds_op_log_threshold); + } + if (changed.count("mds_op_history_size") || changed.count("mds_op_history_duration")) { + op_tracker.set_history_size_and_duration(conf->mds_op_history_size, conf->mds_op_history_duration); + } + if (changed.count("mds_enable_op_tracker")) { + op_tracker.set_tracking(conf->mds_enable_op_tracker); + } + if (changed.count("clog_to_monitors") || + changed.count("clog_to_syslog") || + changed.count("clog_to_syslog_level") || + changed.count("clog_to_syslog_facility") || + changed.count("clog_to_graylog") || + changed.count("clog_to_graylog_host") || + changed.count("clog_to_graylog_port") || + changed.count("host") || + changed.count("fsid")) { + update_log_config(); + } + + finisher->queue(new FunctionContext([this, changed](int r) { + std::scoped_lock lock(mds_lock); + + if (changed.count("mds_log_pause") && !g_conf()->mds_log_pause) { + mdlog->kick_submitter(); + } + sessionmap.handle_conf_change(changed); + server->handle_conf_change(changed); + mdcache->handle_conf_change(changed, *mdsmap); + purge_queue.handle_conf_change(changed, *mdsmap); + })); +} diff --git a/src/mds/MDSRank.h b/src/mds/MDSRank.h index 0575c3a37797c..429a6ba191cdf 100644 --- a/src/mds/MDSRank.h +++ b/src/mds/MDSRank.h @@ -231,15 +231,6 @@ class MDSRank { void handle_write_error(int err); - void handle_conf_change(const ConfigProxy& conf, - const std::set &changed) - { - sessionmap.handle_conf_change(conf, changed); - server->handle_conf_change(conf, changed); - mdcache->handle_conf_change(conf, changed, *mdsmap); - purge_queue.handle_conf_change(conf, changed, *mdsmap); - } - void update_mlogger(); protected: // Flag to indicate we entered shutdown: anyone seeing this to be true @@ -311,7 +302,6 @@ class MDSRank { void create_logger(); public: - void queue_waiter(MDSContext *c) { finished_queue.push_back(c); progress_thread.signal(); @@ -614,7 +604,7 @@ private: * the service/dispatcher stuff like init/shutdown that subsystems should * never touch. */ -class MDSRankDispatcher : public MDSRank +class MDSRankDispatcher : public MDSRank, public md_config_obs_t { public: void init(); @@ -626,6 +616,9 @@ public: void handle_osd_map(); void update_log_config(); + const char** get_tracked_conf_keys() const final; + void handle_conf_change(const ConfigProxy& conf, const std::set& changed) override; + bool handle_command( const cmdmap_t &cmdmap, const cref_t &m, diff --git a/src/mds/Migrator.cc b/src/mds/Migrator.cc index b5579dcb15d97..0551f37b134ae 100644 --- a/src/mds/Migrator.cc +++ b/src/mds/Migrator.cc @@ -3600,14 +3600,12 @@ Migrator::Migrator(MDSRank *m, MDCache *c) : mds(m), cache(c) { inject_session_race = g_conf().get_val("mds_inject_migrator_session_race"); } -void Migrator::handle_conf_change(const ConfigProxy& conf, - const std::set &changed, - const MDSMap &mds_map) +void Migrator::handle_conf_change(const std::set& changed, const MDSMap& mds_map) { if (changed.count("mds_max_export_size")) max_export_size = g_conf().get_val("mds_max_export_size"); if (changed.count("mds_inject_migrator_session_race")) { - inject_session_race = conf.get_val("mds_inject_migrator_session_race"); + inject_session_race = g_conf().get_val("mds_inject_migrator_session_race"); dout(0) << "mds_inject_migrator_session_race is " << inject_session_race << dendl; } } diff --git a/src/mds/Migrator.h b/src/mds/Migrator.h index b4d8bc1318023..a0fa4eafc67a0 100644 --- a/src/mds/Migrator.h +++ b/src/mds/Migrator.h @@ -102,9 +102,7 @@ public: // -- cons -- Migrator(MDSRank *m, MDCache *c); - void handle_conf_change(const ConfigProxy& conf, - const std::set &changed, - const MDSMap &mds_map); + void handle_conf_change(const std::set& changed, const MDSMap& mds_map); protected: struct export_base_t { diff --git a/src/mds/PurgeQueue.cc b/src/mds/PurgeQueue.cc index 882fce5488129..aa9c7f7e104bc 100644 --- a/src/mds/PurgeQueue.cc +++ b/src/mds/PurgeQueue.cc @@ -673,9 +673,7 @@ void PurgeQueue::update_op_limit(const MDSMap &mds_map) } } -void PurgeQueue::handle_conf_change(const ConfigProxy& conf, - const std::set &changed, - const MDSMap &mds_map) +void PurgeQueue::handle_conf_change(const std::set& changed, const MDSMap& mds_map) { if (changed.count("mds_max_purge_ops") || changed.count("mds_max_purge_ops_per_pg")) { @@ -686,7 +684,7 @@ void PurgeQueue::handle_conf_change(const ConfigProxy& conf, // We might have gone from zero to a finite limit, so // might need to kick off consume. dout(4) << "maybe start work again (max_purge_files=" - << conf->mds_max_purge_files << dendl; + << g_conf()->mds_max_purge_files << dendl; finisher.queue(new FunctionContext([this](int r){ std::lock_guard l(lock); _consume(); diff --git a/src/mds/PurgeQueue.h b/src/mds/PurgeQueue.h index 40b1f67d442a4..c1d2de46eef3f 100644 --- a/src/mds/PurgeQueue.h +++ b/src/mds/PurgeQueue.h @@ -211,9 +211,7 @@ public: void update_op_limit(const MDSMap &mds_map); - void handle_conf_change(const ConfigProxy& conf, - const std::set &changed, - const MDSMap &mds_map); + void handle_conf_change(const std::set& changed, const MDSMap& mds_map); PurgeQueue( CephContext *cct_, diff --git a/src/mds/Server.cc b/src/mds/Server.cc index 11752380450c5..92ad6e8a5c9de 100644 --- a/src/mds/Server.cc +++ b/src/mds/Server.cc @@ -1081,8 +1081,7 @@ void Server::evict_cap_revoke_non_responders() { } } -void Server::handle_conf_change(const ConfigProxy& conf, - const std::set &changed) { +void Server::handle_conf_change(const std::set& changed) { if (changed.count("mds_cap_revoke_eviction_timeout")) { cap_revoke_eviction_timeout = g_conf().get_val("mds_cap_revoke_eviction_timeout"); dout(20) << __func__ << " cap revoke eviction timeout changed to " diff --git a/src/mds/Server.h b/src/mds/Server.h index c7f159adcf307..a5296a0a430f8 100644 --- a/src/mds/Server.h +++ b/src/mds/Server.h @@ -343,8 +343,7 @@ public: bool finish_mdr); void evict_cap_revoke_non_responders(); - void handle_conf_change(const ConfigProxy& conf, - const std::set &changed); + void handle_conf_change(const std::set& changed); private: void reply_client_request(MDRequestRef& mdr, const ref_t &reply); diff --git a/src/mds/SessionMap.cc b/src/mds/SessionMap.cc index bd27ed7ac1587..23b24ba387f33 100644 --- a/src/mds/SessionMap.cc +++ b/src/mds/SessionMap.cc @@ -1019,8 +1019,7 @@ void SessionMap::hit_session(Session *session) { session->hit_session(); } -void SessionMap::handle_conf_change(const ConfigProxy &conf, - const std::set &changed) +void SessionMap::handle_conf_change(const std::set& changed) { auto apply_to_open_sessions = [this](auto f) { if (auto it = by_state.find(Session::STATE_OPEN); it != by_state.end()) { diff --git a/src/mds/SessionMap.h b/src/mds/SessionMap.h index 89fa924a0d10f..61ca7aeb48452 100644 --- a/src/mds/SessionMap.h +++ b/src/mds/SessionMap.h @@ -794,8 +794,7 @@ private: public: void hit_session(Session *session); - void handle_conf_change(const ConfigProxy &conf, - const std::set &changed); + void handle_conf_change(const std::set &changed); }; std::ostream& operator<<(std::ostream &out, const Session &s); -- 2.39.5