From: Jianpeng Ma <jianpeng.ma@intel.com>
Date: Wed, 16 Dec 2020 00:20:49 +0000 (+0800)
Subject: os/bluestore: Make OpSequencer has own deferred_lock.
X-Git-Tag: v16.1.0~150^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=e36f368e438ef9c63d35e3765be312bbeffda8dc;p=ceph.git

os/bluestore: Make OpSequencer has own deferred_lock.

For 4k randwrite workload & Using perf script futex-contention in
60second, i found:
>bstore_mempool[69278] lock 7fdc0131f3b8 contended 5 times, 2043 avg ns
>bstore_kv_final[69277] lock 7fdc0131f3b8 contended 41835 times, 1285 avg ns
>bstore_aio[69226] lock 7fdc0131f3b8 contended 62294 times, 1217 avg ns

deferred_lock has serious contented. This becasue deferred_lock is a
globla lock. So i add defferred_lock in OpSequencer and make global
deferred_lock only protect deferred_queue.

After this change:
>bstore_kv_final[75810] lock 7f72f931f3b8 contended 1074 times, 1692 avg ns
>bstore_aio[75759] lock 7f72f931f3b8 contended 1491 times, 1133 avg ns

Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>
---

diff --git a/src/common/Throttle.cc b/src/common/Throttle.cc
index f25fa7cbe6ed..b8b4783dce30 100644
--- a/src/common/Throttle.cc
+++ b/src/common/Throttle.cc
@@ -191,25 +191,31 @@ bool Throttle::get_or_fail(int64_t c)
   }
 
   assert (c >= 0);
-  std::lock_guard l(lock);
-  if (_should_wait(c) || !conds.empty()) {
-    ldout(cct, 10) << "get_or_fail " << c << " failed" << dendl;
-    if (logger) {
-      logger->inc(l_throttle_get_or_fail_fail);
+  bool result = false;
+  {
+    std::lock_guard l(lock);
+    if (_should_wait(c) || !conds.empty()) {
+      ldout(cct, 10) << "get_or_fail " << c << " failed" << dendl;
+      result = false;
+    } else {
+      ldout(cct, 10) << "get_or_fail " << c << " success (" << count.load()
+	<< " -> " << (count.load() + c) << ")" << dendl;
+      count += c;
+      result = true;
     }
-    return false;
-  } else {
-    ldout(cct, 10) << "get_or_fail " << c << " success (" << count.load()
-		   << " -> " << (count.load() + c) << ")" << dendl;
-    count += c;
-    if (logger) {
+  }
+
+  if (logger) {
+    if (result) {
       logger->inc(l_throttle_get_or_fail_success);
       logger->inc(l_throttle_get);
       logger->inc(l_throttle_get_sum, c);
       logger->set(l_throttle_val, count);
+    } else {
+      logger->inc(l_throttle_get_or_fail_fail);
     }
-    return true;
   }
+  return result;
 }
 
 int64_t Throttle::put(int64_t c)
@@ -222,19 +228,22 @@ int64_t Throttle::put(int64_t c)
   ceph_assert(c >= 0);
   ldout(cct, 10) << "put " << c << " (" << count.load() << " -> "
 		 << (count.load()-c) << ")" << dendl;
-  std::lock_guard l(lock);
-  if (c) {
-    if (!conds.empty())
-      conds.front().notify_one();
-    // if count goes negative, we failed somewhere!
-    ceph_assert(count >= c);
-    count -= c;
-    if (logger) {
-      logger->inc(l_throttle_put);
-      logger->inc(l_throttle_put_sum, c);
-      logger->set(l_throttle_val, count);
+  {
+    std::lock_guard l(lock);
+    if (c) {
+      if (!conds.empty())
+	conds.front().notify_one();
+      // if count goes negative, we failed somewhere!
+      ceph_assert(count >= c);
+      count -= c;
     }
   }
+  if (logger) {
+    logger->inc(l_throttle_put);
+    logger->inc(l_throttle_put_sum, c);
+    logger->set(l_throttle_val, count);
+  }
+
   return count;
 }
 
diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 468d846ff487..ae26d5943173 100644
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -12159,6 +12159,8 @@ void BlueStore::_deferred_queue(TransContext *txc)
     txc->osr->deferred_lock.lock();
     ++deferred_queue_size;
     txc->osr->deferred_pending = tmp;
+    // condition "tmp->txcs.size() == 1" mean deferred_pending was originally empty.
+    // So we should add osr into deferred_queue.
     if (!txc->osr->deferred_running && (tmp->txcs.size() == 1)) {
       deferred_lock.lock();
       deferred_queue.push_back(*txc->osr);