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>
(cherry picked from commit 5ade254c15d0b9f96cdd5cd2d5f7861ef2232812)

 Conflicts:
	src/mds/MDSRank.cc
	src/mds/MDSRank.h

diff --git a/src/mds/MDSDaemon.h b/src/mds/MDSDaemon.h
index d92658934bd85227c525a75dafcd8d25f5871dca..97162cc88559cca87cefbb58f20ea0f5520b700e 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 f085e266979c0ed25abce20050606fa9a4e3bdf4..11af941fcc1e38ca6d4bf5a864dfb037ba91a010 100644 (file)
--- a/src/mds/MDSRank.cc
+++ b/src/mds/MDSRank.cc
@@ -477,9 +477,9 @@ private:
 MDSRank::MDSRank(
     mds_rank_t whoami_,
     std::string fs_name_,
-    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,
@@ -3612,9 +3612,9 @@ bool MDSRank::evict_client(int64_t session_id,
 MDSRankDispatcher::MDSRankDispatcher(
     mds_rank_t whoami_,
     std::string fs_name_,
-    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 7ec785c14ffccddaed7c1e7a0a974408d0ca6e4b..2a4b9928158a80d36c3216af4268a0dd5b1c21eb 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"
 
@@ -165,9 +167,9 @@ class MDSRank {
     MDSRank(
         mds_rank_t whoami_,
        std::string fs_name_,
-        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,
@@ -369,7 +371,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.
@@ -378,7 +380,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 */
 
@@ -431,7 +433,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;
@@ -661,9 +663,9 @@ public:
   MDSRankDispatcher(
       mds_rank_t whoami_,
       std::string fs_name,
-      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,