mds: switch mds_lock to fair mutex

The implementations of the Mutex (e.g. std::mutex in C++) do not
guarantee fairness, they do not guarantee that the lock will be
acquired by threads in the order that they called the lock().

In most case this works well, but in corner case in the Finisher
thread in mds daemon, which may call more than one complete()s
once the mdlog flushing succeeds, after the mdlog flushing is done
it will call the queued complete callbacks and the Finisher thread
could always successfully acquire the mds_lock in successive
complete callbakcs even there may have other threads already being
stuck waiting the mds_lock. This will make the other threads starve
and if they are client's requests, it will cause several or even
tens of seconds long delay for user's operations.

This will switch the mds_lock to fair mutex and it could make sure
that the all the mds_lock waiters are in FIFO order and the Finisher
thread won't hold the mds_lock that long.

At the same time, if the finisher thread has many completes needed
to run the fair mutex could guarantee that the finisher won't be
scheduled out due to fair mutex unlock() if no any other mds_lock
waiter queued.

Fixes: https://tracker.ceph.com/issues/51722
Signed-off-by: Xiubo Li <xiubli@redhat.com>

diff --git a/src/mds/MDSDaemon.h b/src/mds/MDSDaemon.h
index b0491f61caf70248749bbd4479d1ff0f35681099..1fe872ba9675b4b2ce3709e2c0e0e1e8308f0651 100644 (file)
--- a/src/mds/MDSDaemon.h
+++ b/src/mds/MDSDaemon.h
@@ -25,6 +25,7 @@
 
 #include "common/LogClient.h"
 #include "common/ceph_mutex.h"
+#include "common/fair_mutex.h"
 #include "common/Timer.h"
 #include "include/Context.h"
 #include "include/types.h"
@@ -72,10 +73,10 @@ class MDSDaemon : public Dispatcher {
    * also check the `stopping` flag.  If stopping is true, you
    * must either do nothing and immediately drop the lock, or
    * never drop the lock again (i.e. call respawn()) */
-  ceph::mutex mds_lock = ceph::make_mutex("MDSDaemon::mds_lock");
+  ceph::fair_mutex mds_lock{"MDSDaemon::mds_lock"};
   bool stopping = false;
 
-  SafeTimer    timer;
+  class CommonSafeTimer<ceph::fair_mutex> timer;
   std::string gss_ktfile_client{};
 
   int orig_argc;

diff --git a/src/mds/MDSRank.cc b/src/mds/MDSRank.cc
index 2b40cfb338950d219b82603954ac6c8c041542b5..91d58a19df5db77a6e1c4e90356bd72be076d559 100644 (file)
--- a/src/mds/MDSRank.cc
+++ b/src/mds/MDSRank.cc
@@ -482,9 +482,9 @@ private:
 
 MDSRank::MDSRank(
     mds_rank_t whoami_,
-    ceph::mutex &mds_lock_,
+    ceph::fair_mutex &mds_lock_,
     LogChannelRef &clog_,
-    SafeTimer &timer_,
+    CommonSafeTimer<ceph::fair_mutex> &timer_,
     Beacon &beacon_,
     std::unique_ptr<MDSMap>& mdsmap_,
     Messenger *msgr,
@@ -3622,9 +3622,9 @@ bool MDSRank::evict_client(int64_t session_id,
 
 MDSRankDispatcher::MDSRankDispatcher(
     mds_rank_t whoami_,
-    ceph::mutex &mds_lock_,
+    ceph::fair_mutex &mds_lock_,
     LogChannelRef &clog_,
-    SafeTimer &timer_,
+    CommonSafeTimer<ceph::fair_mutex> &timer_,
     Beacon &beacon_,
     std::unique_ptr<MDSMap> &mdsmap_,
     Messenger *msgr,

diff --git a/src/mds/MDSRank.h b/src/mds/MDSRank.h
index acdf11ee4c0484359a66121cfc482e2b9da23323..6ba2001932867131206afad769053de531254266 100644 (file)
--- a/src/mds/MDSRank.h
+++ b/src/mds/MDSRank.h
@@ -22,7 +22,9 @@
 #include "common/DecayCounter.h"
 #include "common/LogClient.h"
 #include "common/Timer.h"
+#include "common/fair_mutex.h"
 #include "common/TrackedOp.h"
+#include "common/ceph_mutex.h"
 
 #include "include/common_fwd.h"
 
@@ -164,9 +166,9 @@ class MDSRank {
 
     MDSRank(
         mds_rank_t whoami_,
-        ceph::mutex &mds_lock_,
+        ceph::fair_mutex &mds_lock_,
         LogChannelRef &clog_,
-        SafeTimer &timer_,
+        CommonSafeTimer<ceph::fair_mutex> &timer_,
         Beacon &beacon_,
         std::unique_ptr<MDSMap> & mdsmap_,
         Messenger *msgr,
@@ -367,7 +369,7 @@ class MDSRank {
     // Reference to global MDS::mds_lock, so that users of MDSRank don't
     // carry around references to the outer MDS, and we can substitute
     // a separate lock here in future potentially.
-    ceph::mutex &mds_lock;
+    ceph::fair_mutex &mds_lock;
 
     // Reference to global cluster log client, just to avoid initialising
     // a separate one here.
@@ -376,7 +378,7 @@ class MDSRank {
     // Reference to global timer utility, because MDSRank and MDSDaemon
     // currently both use the same mds_lock, so it makes sense for them
     // to share a timer.
-    SafeTimer &timer;
+    CommonSafeTimer<ceph::fair_mutex> &timer;
 
     std::unique_ptr<MDSMap> &mdsmap; /* MDSDaemon::mdsmap */
 
@@ -429,7 +431,7 @@ class MDSRank {
       void signal() {cond.notify_all();}
       private:
       MDSRank *mds;
-      ceph::condition_variable cond;
+      std::condition_variable_any cond;
     } progress_thread;
 
     class C_MDS_StandbyReplayRestart;
@@ -657,9 +659,9 @@ class MDSRankDispatcher : public MDSRank, public md_config_obs_t
 public:
   MDSRankDispatcher(
       mds_rank_t whoami_,
-      ceph::mutex &mds_lock_,
+      ceph::fair_mutex &mds_lock_,
       LogChannelRef &clog_,
-      SafeTimer &timer_,
+      CommonSafeTimer<ceph::fair_mutex> &timer_,
       Beacon &beacon_,
       std::unique_ptr<MDSMap> &mdsmap_,
       Messenger *msgr,