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>

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 1e884075220f4028bd8e894fc713fa733a12801b..bd55e0410012615cbfbcd95d6d16c57ac57894d2 100755 (executable)
--- 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 15bf6a9cd8dbc43b8a6177f5e256d2299e30ccdb..1f6cdb1dee5d50d5040766c7f236012d79905cd6 100644 (file)
--- 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 52339c5d816065f8cbafaaa72f7a61a9f11c6b1e..5199131e25109a183e013324985f84981fdeef85 100644 (file)
--- 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;