From: Max Kellermann <max.kellermann@ionos.com>
Date: Fri, 11 Oct 2024 22:35:13 +0000 (+0200)
Subject: mds/MDSDaemon: unlock `mds_lock` while shutting down Beacon and others
X-Git-Tag: testing/wip-jcollin-testing-20251001.014437-squid~1^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=aee88b701914cfdfa15558ad6d74850da734bc30;p=ceph-ci.git

mds/MDSDaemon: unlock `mds_lock` while shutting down Beacon and others

This fixes a deadlock bug during MDS shutdown:

- the "signal_handler" thread receives the shutdown signal and invokes
  MDSDaemon::suicide() while holding `mds_lock`

- MDSDaemon::suicide() invokes Beacon::send_and_wait() while still
  holding `mds_lock`

- meanwhile, all "ms_dispatch" threads get stuck waiting for
  `mds_lock`, for example in MDCache::upkeep_main() or
  MDSDaemon::ms_dispatch2()

- Beacon::send_and_wait() waits for a `MSG_MDS_BEACON` packet to be
  dispatched (via `cvar` with a timeout)

At this point, even if a `MSG_MDS_BEACON` packet is received by one of
the worker threads, they will put it in the `DispatchQueue`, but no
dispatcher thread will be able to handle it because they are all
stuck.  The cvar.wait_for() call in Beacon::send_and_wait() will
therefore time out and the `MSG_MDS_BEACON` will never be processed.

The proper solution is to unlock `mds_lock` to avoid the dispatchers
from getting stuck.  And in general, we should be holding a lock
strictly only when it is needed and never do blocking calls while
holding a lock.

Fixes: https://tracker.ceph.com/issues/68760
Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
(cherry picked from commit c0fedb52fa61cd4a847f5be7eb66629f9b7b6ab7)
---

diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc
index 60fb3a87422..a06810621bc 100644
--- a/src/mds/MDSDaemon.cc
+++ b/src/mds/MDSDaemon.cc
@@ -899,6 +899,13 @@ void MDSDaemon::suicide()
   // to wait for us to go laggy. Only do this if we're actually in the MDSMap,
   // because otherwise the MDSMonitor will drop our message.
   beacon.set_want_state(*mdsmap, MDSMap::STATE_DNE);
+
+  /* Unlock the mds_lock while waiting for beacon ACK to avoid a
+   * deadlock with the dispatcher thread which may try to acquire
+   * mds_lock, preventing it from receiving the beacon ACK.
+   */
+  mds_lock.unlock();
+
   if (!mdsmap->is_dne_gid(mds_gid_t(monc->get_global_id()))) {
     beacon.send_and_wait(1);
   }
@@ -907,6 +914,8 @@ void MDSDaemon::suicide()
   if (mgrc.is_initialized())
     mgrc.shutdown();
 
+  mds_lock.lock();
+
   if (mds_rank) {
     mds_rank->shutdown();
   } else {