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 <pdonnell@redhat.com>

diff --git a/src/mds/MDBalancer.cc b/src/mds/MDBalancer.cc
index 2cb109a4e11853526040e2eee4f07ec88f79381b..375866460cc73e73ccd0b79a5aaa3b56556d6662 100644 (file)
--- 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<int64_t>("mds_bal_fragment_interval");
 }
 
-void MDBalancer::handle_conf_change(const ConfigProxy& conf,
-                                   const std::set <std::string> &changed,
-                                   const MDSMap &mds_map)
+void MDBalancer::handle_conf_change(const std::set<std::string>& changed, const MDSMap& mds_map)
 {
   if (changed.count("mds_bal_fragment_dirs"))
     bal_fragment_dirs = g_conf().get_val<bool>("mds_bal_fragment_dirs");

diff --git a/src/mds/MDBalancer.h b/src/mds/MDBalancer.h
index ce48c6febaaab2b86d698ba59a58ebd9f7059684..18661f7bed768e08b2b4579608b8905b4a5546fb 100644 (file)
--- 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 <std::string> &changed,
-                          const MDSMap &mds_map);
+  void handle_conf_change(const std::set<std::string>& changed, const MDSMap& mds_map);
 
   int proc_message(const cref_t<Message> &m);
 

diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
index feaa71289a1beb49ddc8d660ec8cd0ffa8ad2fb4..ca3cfb4b2f49ff0bef49c3ac8735765b0749a9ca 100644 (file)
--- 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 <std::string> &changed,
-                                 const MDSMap &mdsmap)
+void MDCache::handle_conf_change(const std::set<std::string>& changed, const MDSMap& mdsmap)
 {
   if (changed.count("mds_cache_size"))
     cache_inode_limit = g_conf().get_val<int64_t>("mds_cache_size");
@@ -216,8 +214,8 @@ void MDCache::handle_conf_change(const ConfigProxy& conf,
     trim_counter = DecayCounter(g_conf().get_val<double>("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 9898fc37c0667b234474bcbd8f87041509013c83..d6340f0ad5f53b70874e04a77c27e5e046aff05d 100644 (file)
--- 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 <std::string> &changed,
-                          const MDSMap &mds_map);
+  void handle_conf_change(const std::set<std::string>& changed, const MDSMap& mds_map);
   
   // debug
   void log_stat();

diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc
index 6d3cad07b859d1861c878680ecd2d3ceb2252313..cb410a4b45b889f8a90c02501e9a750e225bec49 100644 (file)
--- 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 <std::string> &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 4603f0512ee75e35cdb729d74bfa8700e32239f4..13b29d19d6b59c865f62b93cf3dbe44d7519b67b 100644 (file)
--- 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 <std::string> &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 f683a7bdd1179ea4538040616f2cdc2dfb27e8f0..76fe4ce3d88a335df7eee6b457cc295e9208c3b7 100644 (file)
--- 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<std::string>& 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 0575c3a37797cdd500069a8ca67580929df2e5d0..429a6ba191cdf48efab4df8c8270f939ef39c331 100644 (file)
--- 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 <std::string> &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<std::string>& changed) override;
+
   bool handle_command(
     const cmdmap_t &cmdmap,
     const cref_t<MCommand> &m,

diff --git a/src/mds/Migrator.cc b/src/mds/Migrator.cc
index b5579dcb15d977d83d47e25e226542f241b29e8e..0551f37b134ae3be01808e3b6e0a8d6e399799d7 100644 (file)
--- 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<bool>("mds_inject_migrator_session_race");
 }
 
-void Migrator::handle_conf_change(const ConfigProxy& conf,
-                                  const std::set <std::string> &changed,
-                                  const MDSMap &mds_map)
+void Migrator::handle_conf_change(const std::set<std::string>& changed, const MDSMap& mds_map)
 {
   if (changed.count("mds_max_export_size"))
     max_export_size = g_conf().get_val<Option::size_t>("mds_max_export_size");
   if (changed.count("mds_inject_migrator_session_race")) {
-    inject_session_race = conf.get_val<bool>("mds_inject_migrator_session_race");
+    inject_session_race = g_conf().get_val<bool>("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 b4d8bc1318023025e54dadacdce345ebc061d37b..a0fa4eafc67a07b748b689c8ddad27f30276ea8b 100644 (file)
--- 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 <std::string> &changed,
-                          const MDSMap &mds_map);
+  void handle_conf_change(const std::set<std::string>& changed, const MDSMap& mds_map);
 
 protected:
   struct export_base_t {

diff --git a/src/mds/PurgeQueue.cc b/src/mds/PurgeQueue.cc
index 882fce5488129c30ea58d4220796b92464b6fa69..aa9c7f7e104bcf52e7cf147e8d5b9bfd25be5f24 100644 (file)
--- 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 <std::string> &changed,
-                             const MDSMap &mds_map)
+void PurgeQueue::handle_conf_change(const std::set<std::string>& 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 40b1f67d442a49fb8912858d5ff7ad56f749b5ab..c1d2de46eef3f6999137d183742397833f29391e 100644 (file)
--- 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 <std::string> &changed,
-                          const MDSMap &mds_map);
+  void handle_conf_change(const std::set<std::string>& changed, const MDSMap& mds_map);
 
   PurgeQueue(
       CephContext *cct_,

diff --git a/src/mds/Server.cc b/src/mds/Server.cc
index 11752380450c5b3963c8067e90c19ec3df8aa157..92ad6e8a5c9de2e6cf3e195e6abee4bc44dc27a0 100644 (file)
--- 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 <std::string> &changed) {
+void Server::handle_conf_change(const std::set<std::string>& changed) {
   if (changed.count("mds_cap_revoke_eviction_timeout")) {
     cap_revoke_eviction_timeout = g_conf().get_val<double>("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 c7f159adcf307e599c9baa165381078ba7ae0637..a5296a0a430f848ea9dccf1b4058fc22953d57d9 100644 (file)
--- 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 <std::string> &changed);
+  void handle_conf_change(const std::set<std::string>& changed);
 
 private:
   void reply_client_request(MDRequestRef& mdr, const ref_t<MClientReply> &reply);

diff --git a/src/mds/SessionMap.cc b/src/mds/SessionMap.cc
index bd27ed7ac158741d2d0bbdf33d605d59d6f564cd..23b24ba387f330fa8e90d8b0eec6c48736282b29 100644 (file)
--- 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 <std::string> &changed)
+void SessionMap::handle_conf_change(const std::set<std::string>& 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 89fa924a0d10f24bc2404600a0dd4fdef6702fd5..61ca7aeb4845246144ee5f557743f22b4895ee8b 100644 (file)
--- 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 <std::string> &changed);
+  void handle_conf_change(const std::set <std::string> &changed);
 };
 
 std::ostream& operator<<(std::ostream &out, const Session &s);