librbd/cache/pwl: move write_image_cache_state() out of m_lock

periodic_stats() will get m_lock, then get owner_lock. It is opposite
to the lock getting order of SnapshotCreateRequest::handle_notify_quiesce().
move write_image_cache_state() out of m_lock scope. After calling
update_image_cache_state(), and m_lock auto released, then call
write_image_cache_state() to update state in osds.

Fixes: https://tracker.ceph.com/issues/56703
Signed-off-by: Yin Congmin <congmin.yin@intel.com>
(cherry picked from commit a0e2868d9473a9120c4c5d478f6a592859ce1aec)

diff --git a/src/librbd/cache/pwl/AbstractWriteLog.cc b/src/librbd/cache/pwl/AbstractWriteLog.cc
index 49f4161effe07fbf2874325df16ae59d135be3e2..72fcf4fe95d466188f272dc7aff9f905db992ea2 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.cc
+++ b/src/librbd/cache/pwl/AbstractWriteLog.cc
@@ -314,8 +314,11 @@ void AbstractWriteLog<I>::log_perf() {
 
 template <typename I>
 void AbstractWriteLog<I>::periodic_stats() {
-  std::lock_guard locker(m_lock);
-  update_image_cache_state();
+  {
+    std::lock_guard locker(m_lock);
+    update_image_cache_state();
+  }
+  write_image_cache_state();
   ldout(m_image_ctx.cct, 5) << "STATS: m_log_entries=" << m_log_entries.size()
                             << ", m_dirty_log_entries=" << m_dirty_log_entries.size()
                             << ", m_free_log_entries=" << m_free_log_entries
@@ -570,15 +573,15 @@ void AbstractWriteLog<I>::pwl_init(Context *on_finish, DeferredContexts &later)
 }
 
 template <typename I>
-void AbstractWriteLog<I>::update_image_cache_state() {
+void AbstractWriteLog<I>::write_image_cache_state() {
   using klass = AbstractWriteLog<I>;
   Context *ctx = util::create_context_callback<
-                 klass, &klass::handle_update_image_cache_state>(this);
-  update_image_cache_state(ctx);
+                 klass, &klass::handle_write_image_cache_state>(this);
+  m_cache_state->write_image_cache_state(ctx);
 }
 
 template <typename I>
-void AbstractWriteLog<I>::update_image_cache_state(Context *on_finish) {
+void AbstractWriteLog<I>::update_image_cache_state() {
   ldout(m_image_ctx.cct, 10) << dendl;
 
   ceph_assert(ceph_mutex_is_locked_by_me(m_lock));
@@ -593,11 +596,10 @@ void AbstractWriteLog<I>::update_image_cache_state(Context *on_finish) {
   m_cache_state->hit_bytes = m_perfcounter->get(l_librbd_pwl_rd_hit_bytes);
   m_cache_state->miss_bytes = m_perfcounter->get(l_librbd_pwl_rd_bytes) -
       m_cache_state->hit_bytes;
-  m_cache_state->write_image_cache_state(on_finish);
 }
 
 template <typename I>
-void AbstractWriteLog<I>::handle_update_image_cache_state(int r) {
+void AbstractWriteLog<I>::handle_write_image_cache_state(int r) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 10) << "r=" << r << dendl;
 
@@ -622,8 +624,11 @@ void AbstractWriteLog<I>::init(Context *on_finish) {
   Context *ctx = new LambdaContext(
     [this, on_finish](int r) {
       if (r >= 0) {
-        std::lock_guard locker(m_lock);
-        update_image_cache_state(on_finish);
+        {
+          std::lock_guard locker(m_lock);
+          update_image_cache_state();
+        }
+        m_cache_state->write_image_cache_state(on_finish);
       } else {
         on_finish->complete(r);
       }
@@ -654,14 +659,17 @@ void AbstractWriteLog<I>::shut_down(Context *on_finish) {
       Context *next_ctx = override_ctx(r, ctx);
       periodic_stats();
 
-      std::lock_guard locker(m_lock);
-      check_image_cache_state_clean();
-      m_wake_up_enabled = false;
-      m_log_entries.clear();
-      m_cache_state->clean = true;
-      m_cache_state->empty = true;
-      remove_pool_file();
-      update_image_cache_state(next_ctx);
+      {
+        std::lock_guard locker(m_lock);
+        check_image_cache_state_clean();
+        m_wake_up_enabled = false;
+        m_log_entries.clear();
+        m_cache_state->clean = true;
+        m_cache_state->empty = true;
+        remove_pool_file();
+        update_image_cache_state();
+      }
+      m_cache_state->write_image_cache_state(next_ctx);
     });
   ctx = new LambdaContext(
     [this, ctx](int r) {
@@ -1305,6 +1313,7 @@ void AbstractWriteLog<I>::complete_op_log_entries(GenericLogOperations &&ops,
 {
   GenericLogEntries dirty_entries;
   int published_reserves = 0;
+  bool need_update_state = false;
   ldout(m_image_ctx.cct, 20) << __func__ << ": completing" << dendl;
   for (auto &op : ops) {
     utime_t now = ceph_clock_now();
@@ -1327,6 +1336,7 @@ void AbstractWriteLog<I>::complete_op_log_entries(GenericLogOperations &&ops,
       if (m_cache_state->clean && !this->m_dirty_log_entries.empty()) {
         m_cache_state->clean = false;
         update_image_cache_state();
+        need_update_state = true;
       }
     }
     op->complete(result);
@@ -1342,6 +1352,9 @@ void AbstractWriteLog<I>::complete_op_log_entries(GenericLogOperations &&ops,
                       log_entry->ram_entry.write_bytes);
     m_perfcounter->tinc(l_librbd_pwl_log_op_app_to_cmp_t, now - op->log_append_start_time);
   }
+  if (need_update_state) {
+    write_image_cache_state();
+  }
   // New entries may be flushable
   {
     std::lock_guard locker(m_lock);
@@ -1738,6 +1751,7 @@ void AbstractWriteLog<I>::process_writeback_dirty_entries() {
   bool all_clean = false;
   int flushed = 0;
   bool has_write_entry = false;
+  bool need_update_state = false;
 
   ldout(cct, 20) << "Look for dirty entries" << dendl;
   {
@@ -1760,6 +1774,7 @@ void AbstractWriteLog<I>::process_writeback_dirty_entries() {
         if (!m_cache_state->clean && all_clean) {
           m_cache_state->clean = true;
           update_image_cache_state();
+          need_update_state = true;
         }
         break;
       }
@@ -1791,6 +1806,9 @@ void AbstractWriteLog<I>::process_writeback_dirty_entries() {
 
     construct_flush_entries(entries_to_flush, post_unlock, has_write_entry);
   }
+  if (need_update_state) {
+    write_image_cache_state();
+  }
 
   if (all_clean) {
     /* All flushing complete, drain outside lock */
@@ -2005,6 +2023,7 @@ void AbstractWriteLog<I>::flush_dirty_entries(Context *on_finish) {
   bool all_clean;
   bool flushing;
   bool stop_flushing;
+  bool need_update_state = false;
 
   {
     std::lock_guard locker(m_lock);
@@ -2013,9 +2032,13 @@ void AbstractWriteLog<I>::flush_dirty_entries(Context *on_finish) {
     if (!m_cache_state->clean && all_clean && !flushing) {
       m_cache_state->clean = true;
       update_image_cache_state();
+      need_update_state = true;
     }
     stop_flushing = (m_shutting_down);
   }
+  if (need_update_state) {
+    write_image_cache_state();
+  }
 
   if (!flushing && (all_clean || stop_flushing)) {
     /* Complete without holding m_lock */

diff --git a/src/librbd/cache/pwl/AbstractWriteLog.h b/src/librbd/cache/pwl/AbstractWriteLog.h
index 4905edde6e770d1aba52078e916e935bc3a69c04..5fece849abccbadef417a1b028fb7a9d987fb68a 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.h
+++ b/src/librbd/cache/pwl/AbstractWriteLog.h
@@ -233,8 +233,6 @@ private:
   void arm_periodic_stats();
 
   void pwl_init(Context *on_finish, pwl::DeferredContexts &later);
-  void update_image_cache_state(Context *on_finish);
-  void handle_update_image_cache_state(int r);
   void check_image_cache_state_clean();
 
   void flush_dirty_entries(Context *on_finish);
@@ -399,6 +397,8 @@ protected:
     return 0;
   }
   void update_image_cache_state(void);
+  void write_image_cache_state(void);
+  void handle_write_image_cache_state(int r);
 };
 
 } // namespace pwl

diff --git a/src/librbd/cache/pwl/rwl/WriteLog.cc b/src/librbd/cache/pwl/rwl/WriteLog.cc
index 41bdafe5b9e3d148c4819ffd2e9a3506d9d0f7e9..1fdd5d966b08872ed07d1e5cfc281af494a45604 100644 (file)
--- a/src/librbd/cache/pwl/rwl/WriteLog.cc
+++ b/src/librbd/cache/pwl/rwl/WriteLog.cc
@@ -100,26 +100,33 @@ void WriteLog<I>::alloc_op_log_entries(GenericLogOperations &ops)
   TOID(struct WriteLogPoolRoot) pool_root;
   pool_root = POBJ_ROOT(m_log_pool, struct WriteLogPoolRoot);
   struct WriteLogCacheEntry *pmem_log_entries = D_RW(D_RW(pool_root)->log_entries);
+  bool need_update_state = false;
 
   ceph_assert(ceph_mutex_is_locked_by_me(this->m_log_append_lock));
 
-  /* Allocate the (already reserved) log entries */
-  std::lock_guard locker(m_lock);
+  {
+    /* Allocate the (already reserved) log entries */
+    std::lock_guard locker(m_lock);
 
-  for (auto &operation : ops) {
-    uint32_t entry_index = this->m_first_free_entry;
-    this->m_first_free_entry = (this->m_first_free_entry + 1) % this->m_total_log_entries;
-    auto &log_entry = operation->get_log_entry();
-    log_entry->log_entry_index = entry_index;
-    log_entry->ram_entry.entry_index = entry_index;
-    log_entry->cache_entry = &pmem_log_entries[entry_index];
-    log_entry->ram_entry.set_entry_valid(true);
-    m_log_entries.push_back(log_entry);
-    ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl;
+    for (auto &operation : ops) {
+      uint32_t entry_index = this->m_first_free_entry;
+      this->m_first_free_entry = (this->m_first_free_entry + 1) % this->m_total_log_entries;
+      auto &log_entry = operation->get_log_entry();
+      log_entry->log_entry_index = entry_index;
+      log_entry->ram_entry.entry_index = entry_index;
+      log_entry->cache_entry = &pmem_log_entries[entry_index];
+      log_entry->ram_entry.set_entry_valid(true);
+      m_log_entries.push_back(log_entry);
+      ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl;
+    }
+    if (m_cache_state->empty && !m_log_entries.empty()) {
+      m_cache_state->empty = false;
+      this->update_image_cache_state();
+      need_update_state = true;
+    }
   }
-  if (m_cache_state->empty && !m_log_entries.empty()) {
-    m_cache_state->empty = false;
-    this->update_image_cache_state();
+  if (need_update_state) {
+    this->write_image_cache_state();
   }
 }
 
@@ -547,6 +554,7 @@ bool WriteLog<I>::retire_entries(const unsigned long int frees_per_tx) {
     m_perfcounter->hinc(l_librbd_pwl_retire_tx_t_hist, utime_t(tx_end - tx_start).to_nsec(),
         retiring_entries.size());
 
+    bool need_update_state = false;
     /* Update runtime copy of first_valid, and free entries counts */
     {
       std::lock_guard locker(m_lock);
@@ -557,6 +565,7 @@ bool WriteLog<I>::retire_entries(const unsigned long int frees_per_tx) {
       if (!m_cache_state->empty && m_log_entries.empty()) {
         m_cache_state->empty = true;
         this->update_image_cache_state();
+        need_update_state = true;
       }
       for (auto &entry: retiring_entries) {
         if (entry->write_bytes()) {
@@ -573,6 +582,9 @@ bool WriteLog<I>::retire_entries(const unsigned long int frees_per_tx) {
       this->m_alloc_failed_since_retire = false;
       this->wake_up();
     }
+    if (need_update_state) {
+      this->write_image_cache_state();
+    }
   } else {
     ldout(cct, 20) << "Nothing to retire" << dendl;
     return false;

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc
index 2c0dc258b86fb5dc03adf368b42e34dac934a41e..1fee2d13e72e2562d6520dbf5d4dd68285efc726 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.cc
+++ b/src/librbd/cache/pwl/ssd/WriteLog.cc
@@ -531,17 +531,23 @@ void WriteLog<I>::release_ram(std::shared_ptr<GenericLogEntry> log_entry) {
 
 template <typename I>
 void WriteLog<I>::alloc_op_log_entries(GenericLogOperations &ops) {
-  std::lock_guard locker(m_lock);
-
-  for (auto &operation : ops) {
-    auto &log_entry = operation->get_log_entry();
-    log_entry->ram_entry.set_entry_valid(true);
-    m_log_entries.push_back(log_entry);
-    ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl;
+  bool need_update_state = false;
+  {
+    std::lock_guard locker(m_lock);
+    for (auto &operation : ops) {
+      auto &log_entry = operation->get_log_entry();
+      log_entry->ram_entry.set_entry_valid(true);
+      m_log_entries.push_back(log_entry);
+      ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl;
+    }
+    if (m_cache_state->empty && !m_log_entries.empty()) {
+      m_cache_state->empty = false;
+      this->update_image_cache_state();
+      need_update_state = true;
+    }
   }
-  if (m_cache_state->empty && !m_log_entries.empty()) {
-    m_cache_state->empty = false;
-    this->update_image_cache_state();
+  if (need_update_state) {
+    this->write_image_cache_state();
   }
 }
 
@@ -807,6 +813,7 @@ bool WriteLog<I>::retire_entries(const unsigned long int frees_per_tx) {
             allocated_bytes += entry->get_aligned_data_size();
           }
         }
+        bool need_update_state = false;
         {
           std::lock_guard locker(m_lock);
           m_first_valid_entry = first_valid_entry;
@@ -818,6 +825,7 @@ bool WriteLog<I>::retire_entries(const unsigned long int frees_per_tx) {
           if (!m_cache_state->empty && m_log_entries.empty()) {
             m_cache_state->empty = true;
             this->update_image_cache_state();
+            need_update_state = true;
           }
 
           ldout(m_image_ctx.cct, 20)
@@ -832,6 +840,9 @@ bool WriteLog<I>::retire_entries(const unsigned long int frees_per_tx) {
           this->m_alloc_failed_since_retire = false;
           this->wake_up();
         }
+        if (need_update_state) {
+          this->write_image_cache_state();
+        }
 
         this->dispatch_deferred_writes();
         this->process_writeback_dirty_entries();