From 59ad5afebf3dab640cdc8399da640d18cb3f03c4 Mon Sep 17 00:00:00 2001
From: Xiubo Li <xiubli@redhat.com>
Date: Thu, 23 Jul 2020 12:43:44 +0800
Subject: [PATCH] client: add timer_lock support

Do not use the big client_lock for the SafeTimer, this will help
to get rid of the client_lock for the whole tick() function. And
also, will make the SafeTimer members a small ciritical section
separating from the Client.

Fixes: https://tracker.ceph.com/issues/46682
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 src/client/Client.cc     | 47 ++++++++++++++++++++++++++++------------
 src/client/Client.h      |  4 +++-
 src/client/Delegation.cc |  6 ++---
 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 1e884075220f4..bd55e04100126 100755
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -267,7 +267,7 @@ vinodeno_t Client::map_faked_ino(ino_t ino)
 
 Client::Client(Messenger *m, MonClient *mc, Objecter *objecter_)
   : Dispatcher(m->cct),
-    timer(m->cct, client_lock),
+    timer(m->cct, timer_lock, false),
     messenger(m),
     monclient(mc),
     objecter(objecter_),
@@ -607,6 +607,10 @@ void Client::shutdown()
     std::lock_guard l{client_lock};
     ceph_assert(initialized);
     initialized = false;
+  }
+
+  {
+    std::scoped_lock l(timer_lock);
     timer.shutdown();
   }
   objecter_finisher.wait_for_empty();
@@ -5953,7 +5957,7 @@ int Client::subscribe_mdsmap(const std::string &fs_name)
 int Client::mount(const std::string &mount_root, const UserPerm& perms,
 		  bool require_mds, const std::string &fs_name)
 {
-  std::lock_guard lock(client_lock);
+  std::unique_lock lock(client_lock);
 
   if (mounted) {
     ldout(cct, 5) << "already mounted" << dendl;
@@ -5968,7 +5972,9 @@ int Client::mount(const std::string &mount_root, const UserPerm& perms,
     return r;
   }
 
+  lock.unlock();
   tick(); // start tick
+  lock.lock();
   
   if (require_mds) {
     while (1) {
@@ -6166,9 +6172,13 @@ void Client::_unmount(bool abort)
     }
     return mds_requests.empty();
   });
-  if (tick_event)
-    timer.cancel_event(tick_event);
-  tick_event = 0;
+
+  {
+    std::scoped_lock l(timer_lock);
+    if (tick_event)
+      timer.cancel_event(tick_event);
+    tick_event = 0;
+  }
 
   cwd.reset();
 
@@ -6303,22 +6313,27 @@ void Client::flush_cap_releases()
 
 void Client::tick()
 {
+  ldout(cct, 20) << "tick" << dendl;
+
+  {
+    std::scoped_lock l(timer_lock);
+    tick_event = timer.add_event_after(
+      cct->_conf->client_tick_interval,
+      new LambdaContext([this](int) {
+          tick();
+      }));
+  }
+
   if (cct->_conf->client_debug_inject_tick_delay > 0) {
     sleep(cct->_conf->client_debug_inject_tick_delay);
     ceph_assert(0 == cct->_conf.set_val("client_debug_inject_tick_delay", "0"));
     cct->_conf.apply_changes(nullptr);
   }
 
-  ldout(cct, 21) << "tick" << dendl;
-  tick_event = timer.add_event_after(
-    cct->_conf->client_tick_interval,
-    new LambdaContext([this](int) {
-	// Called back via Timer, which takes client_lock for us
-	ceph_assert(ceph_mutex_is_locked_by_me(client_lock));
-	tick();
-      }));
   utime_t now = ceph_clock_now();
 
+  std::lock_guard lock(client_lock);
+
   if (!mounted && !mds_requests.empty()) {
     MetaRequest *req = mds_requests.begin()->second;
     if (req->op_stamp + cct->_conf->client_mount_timeout < now) {
@@ -14773,7 +14788,11 @@ int StandaloneClient::init()
   int r = monclient->init();
   if (r < 0) {
     // need to do cleanup because we're in an intermediate init state
-    timer.shutdown();
+    {
+      std::scoped_lock l(timer_lock);
+      timer.shutdown();
+    }
+
     client_lock.unlock();
     objecter->shutdown();
     objectcacher->stop();
diff --git a/src/client/Client.h b/src/client/Client.h
index 15bf6a9cd8dbc..1f6cdb1dee5d5 100644
--- a/src/client/Client.h
+++ b/src/client/Client.h
@@ -734,6 +734,9 @@ public:
 
   xlist<Inode*> &get_dirty_list() { return dirty_list; }
 
+  /* timer_lock for 'timer' and 'tick_event' */
+  ceph::mutex timer_lock = ceph::make_mutex("Client::timer_lock");
+  Context *tick_event = nullptr;
   SafeTimer timer;
 
   std::unique_ptr<PerfCounters> logger;
@@ -1199,7 +1202,6 @@ private:
   Finisher async_ino_releasor;
   Finisher objecter_finisher;
 
-  Context *tick_event = nullptr;
   utime_t last_cap_renew;
 
   CommandHook m_command_hook;
diff --git a/src/client/Delegation.cc b/src/client/Delegation.cc
index 52339c5d81606..5199131e25109 100644
--- a/src/client/Delegation.cc
+++ b/src/client/Delegation.cc
@@ -16,13 +16,11 @@ public:
     Inode *in = deleg->get_fh()->inode.get();
     Client *client = in->client;
 
-    // Called back via Timer, which takes client_lock for us
-    ceph_assert(ceph_mutex_is_locked_by_me(client->client_lock));
-
     lsubdout(client->cct, client, 0) << __func__ <<
 	  ": delegation return timeout for inode 0x" <<
 	  std::hex << in->ino << ". Forcibly unmounting client. "<<
 	  client << std::dec << dendl;
+    std::scoped_lock l(client->client_lock);
     client->_unmount(false);
   }
 };
@@ -100,6 +98,7 @@ void Delegation::arm_timeout()
 {
   Client *client = fh->inode.get()->client;
 
+  std::scoped_lock l(client->timer_lock);
   if (timeout_event)
     return;
 
@@ -111,6 +110,7 @@ void Delegation::disarm_timeout()
 {
   Client *client = fh->inode.get()->client;
 
+  std::scoped_lock l(client->timer_lock);
   if (!timeout_event)
     return;
 
-- 
2.39.5