librbd/cache/pwl: generate image cache state json under m_lock

The previous commit moved the entirety of write_image_cache_state()
from under m_lock.  This was a step too far because the generated image
cache state json is no longer guaranteed to be consistent.

Arrange for m_lock to still be held during image cache json generation
but released before owner_lock is grabbed.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit ad504b10f60290cc7461ea96eaada1fb3f7639d7)

diff --git a/src/librbd/cache/pwl/AbstractWriteLog.cc b/src/librbd/cache/pwl/AbstractWriteLog.cc
index 616b331f375267768562b230b8d4f2ba6d1638a5..3d686bc04493521462ae7545c32caec2e1710556 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.cc
+++ b/src/librbd/cache/pwl/AbstractWriteLog.cc
@@ -313,11 +313,7 @@ void AbstractWriteLog<I>::log_perf() {
 
 template <typename I>
 void AbstractWriteLog<I>::periodic_stats() {
-  {
-    std::lock_guard locker(m_lock);
-    update_image_cache_state();
-  }
-  write_image_cache_state();
+  std::unique_lock locker(m_lock);
   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
@@ -330,6 +326,9 @@ void AbstractWriteLog<I>::periodic_stats() {
                             << ", m_current_sync_gen=" << m_current_sync_gen
                             << ", m_flushed_sync_gen=" << m_flushed_sync_gen
                             << dendl;
+
+  update_image_cache_state();
+  write_image_cache_state(locker);
 }
 
 template <typename I>
@@ -571,11 +570,11 @@ void AbstractWriteLog<I>::pwl_init(Context *on_finish, DeferredContexts &later)
 }
 
 template <typename I>
-void AbstractWriteLog<I>::write_image_cache_state() {
+void AbstractWriteLog<I>::write_image_cache_state(std::unique_lock<ceph::mutex>& locker) {
   using klass = AbstractWriteLog<I>;
   Context *ctx = util::create_context_callback<
                  klass, &klass::handle_write_image_cache_state>(this);
-  m_cache_state->write_image_cache_state(ctx);
+  m_cache_state->write_image_cache_state(locker, ctx);
 }
 
 template <typename I>
@@ -622,11 +621,9 @@ 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();
-        }
-        m_cache_state->write_image_cache_state(on_finish);
+        std::unique_lock locker(m_lock);
+        update_image_cache_state();
+        m_cache_state->write_image_cache_state(locker, on_finish);
       } else {
         on_finish->complete(r);
       }
@@ -657,17 +654,15 @@ 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();
-      }
-      m_cache_state->write_image_cache_state(next_ctx);
+      std::unique_lock 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(locker, next_ctx);
     });
   ctx = new LambdaContext(
     [this, ctx](int r) {
@@ -1353,7 +1348,8 @@ void AbstractWriteLog<I>::complete_op_log_entries(GenericLogOperations &&ops,
     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();
+    std::unique_lock locker(m_lock);
+    write_image_cache_state(locker);
   }
   // New entries may be flushable
   {
@@ -1815,7 +1811,8 @@ 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();
+    std::unique_lock locker(m_lock);
+    write_image_cache_state(locker);
   }
 
   if (all_clean) {
@@ -2031,21 +2028,17 @@ 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);
+    std::unique_lock locker(m_lock);
     flushing = (0 != m_flush_ops_in_flight);
     all_clean = m_dirty_log_entries.empty();
+    stop_flushing = (m_shutting_down);
     if (!m_cache_state->clean && all_clean && !flushing) {
       m_cache_state->clean = true;
       update_image_cache_state();
-      need_update_state = true;
+      write_image_cache_state(locker);
     }
-    stop_flushing = (m_shutting_down);
-  }
-  if (need_update_state) {
-    write_image_cache_state();
   }
 
   if (!flushing && (all_clean || stop_flushing)) {

diff --git a/src/librbd/cache/pwl/AbstractWriteLog.h b/src/librbd/cache/pwl/AbstractWriteLog.h
index 5fece849abccbadef417a1b028fb7a9d987fb68a..ffe299c37d3e3d2cfc7d088722bf96cb1095189c 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.h
+++ b/src/librbd/cache/pwl/AbstractWriteLog.h
@@ -397,7 +397,7 @@ protected:
     return 0;
   }
   void update_image_cache_state(void);
-  void write_image_cache_state(void);
+  void write_image_cache_state(std::unique_lock<ceph::mutex>& locker);
   void handle_write_image_cache_state(int r);
 };
 

diff --git a/src/librbd/cache/pwl/ImageCacheState.cc b/src/librbd/cache/pwl/ImageCacheState.cc
index a1e3b1bb6cbd55811746e13e384d213d7217a6f9..2bd6e11324975e9c78d40d97b4e799b81c66bdc6 100644 (file)
--- a/src/librbd/cache/pwl/ImageCacheState.cc
+++ b/src/librbd/cache/pwl/ImageCacheState.cc
@@ -58,9 +58,10 @@ bool ImageCacheState<I>::init_from_metadata(json_spirit::mValue& json_root) {
 }
 
 template <typename I>
-void ImageCacheState<I>::write_image_cache_state(Context *on_finish) {
+void ImageCacheState<I>::write_image_cache_state(std::unique_lock<ceph::mutex>& locker,
+                                                 Context *on_finish) {
+  ceph_assert(ceph_mutex_is_locked_by_me(*locker.mutex()));
   stats_timestamp = ceph_clock_now();
-  std::shared_lock owner_lock{m_image_ctx->owner_lock};
   json_spirit::mObject o;
   o["present"] = present;
   o["empty"] = empty;
@@ -80,7 +81,9 @@ void ImageCacheState<I>::write_image_cache_state(Context *on_finish) {
   o["hit_bytes"] = hit_bytes;
   o["miss_bytes"] = miss_bytes;
   std::string image_state_json = json_spirit::write(o);
+  locker.unlock();
 
+  std::shared_lock owner_lock{m_image_ctx->owner_lock};
   ldout(m_image_ctx->cct, 20) << __func__ << " Store state: "
                               << image_state_json << dendl;
   m_plugin_api.execute_image_metadata_set(m_image_ctx, PERSISTENT_CACHE_STATE,

diff --git a/src/librbd/cache/pwl/ImageCacheState.h b/src/librbd/cache/pwl/ImageCacheState.h
index c2fd4b778253f8d89fe0b5430ad2ad1b162a4b78..5be5f73ac1513539db28ddfb881ad029e71b0b0e 100644 (file)
--- a/src/librbd/cache/pwl/ImageCacheState.h
+++ b/src/librbd/cache/pwl/ImageCacheState.h
@@ -63,7 +63,8 @@ public:
   void init_from_config();
   bool init_from_metadata(json_spirit::mValue& json_root);
 
-  void write_image_cache_state(Context *on_finish);
+  void write_image_cache_state(std::unique_lock<ceph::mutex>& locker,
+                               Context *on_finish);
 
   void clear_image_cache_state(Context *on_finish);
 

diff --git a/src/librbd/cache/pwl/rwl/WriteLog.cc b/src/librbd/cache/pwl/rwl/WriteLog.cc
index 55d3f2e32b40dabb6fc80d13d92ba35100563d05..dd623c9adaadf02682664fe9942ab73be61f41c6 100644 (file)
--- a/src/librbd/cache/pwl/rwl/WriteLog.cc
+++ b/src/librbd/cache/pwl/rwl/WriteLog.cc
@@ -99,33 +99,27 @@ 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::unique_lock 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;
-    }
-    if (m_cache_state->empty && !m_log_entries.empty()) {
-      m_cache_state->empty = false;
-      this->update_image_cache_state();
-      need_update_state = true;
-    }
+  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 (need_update_state) {
-    this->write_image_cache_state();
+  if (m_cache_state->empty && !m_log_entries.empty()) {
+    m_cache_state->empty = false;
+    this->update_image_cache_state();
+    this->write_image_cache_state(locker);
   }
 }
 
@@ -583,7 +577,8 @@ bool WriteLog<I>::retire_entries(const unsigned long int frees_per_tx) {
       this->wake_up();
     }
     if (need_update_state) {
-      this->write_image_cache_state();
+      std::unique_lock locker(m_lock);
+      this->write_image_cache_state(locker);
     }
   } else {
     ldout(cct, 20) << "Nothing to retire" << dendl;

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc
index 287ca06d90bf2a46eb7afb7e96e078d98181714b..00626506a3666c9280880897b5288c12fac2629e 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.cc
+++ b/src/librbd/cache/pwl/ssd/WriteLog.cc
@@ -529,23 +529,18 @@ void WriteLog<I>::release_ram(std::shared_ptr<GenericLogEntry> log_entry) {
 
 template <typename I>
 void WriteLog<I>::alloc_op_log_entries(GenericLogOperations &ops) {
-  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;
-    }
+  std::unique_lock 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 (need_update_state) {
-    this->write_image_cache_state();
+  if (m_cache_state->empty && !m_log_entries.empty()) {
+    m_cache_state->empty = false;
+    this->update_image_cache_state();
+    this->write_image_cache_state(locker);
   }
 }
 
@@ -839,7 +834,8 @@ bool WriteLog<I>::retire_entries(const unsigned long int frees_per_tx) {
           this->wake_up();
         }
         if (need_update_state) {
-          this->write_image_cache_state();
+          std::unique_lock locker(m_lock);
+          this->write_image_cache_state(locker);
         }
 
         this->dispatch_deferred_writes();

diff --git a/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc b/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc
index fc372b0d8c7a16a40df83428cb750eef92580a1d..a37f5803897422d6f8d3fbb83feecde104df8443 100644 (file)
--- a/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc
+++ b/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc
@@ -118,10 +118,13 @@ TEST_F(TestMockCacheReplicatedWriteLog, init_state_write) {
   
   image_cache_state.empty = false;
   image_cache_state.clean = false;
+  ceph::mutex lock = ceph::make_mutex("MockImageCacheStateRWL lock");
   MockContextRWL finish_ctx;
   expect_metadata_set(mock_image_ctx);
   expect_context_complete(finish_ctx, 0);
-  image_cache_state.write_image_cache_state(&finish_ctx);
+  std::unique_lock locker(lock);
+  image_cache_state.write_image_cache_state(locker, &finish_ctx);
+  ASSERT_FALSE(locker.owns_lock());
   ASSERT_EQ(0, finish_ctx.wait());
 }
 

diff --git a/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc b/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc
index b1fc4726461a5396516262d4edb83591fb637781..72a44dcc9e1110bed21371a18a5e8b65dc009ff6 100644 (file)
--- a/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc
+++ b/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc
@@ -120,10 +120,13 @@ TEST_F(TestMockCacheSSDWriteLog, init_state_write) {
 
   image_cache_state.empty = false;
   image_cache_state.clean = false;
+  ceph::mutex lock = ceph::make_mutex("MockImageCacheStateSSD lock");
   MockContextSSD finish_ctx;
   expect_metadata_set(mock_image_ctx);
   expect_context_complete(finish_ctx, 0);
-  image_cache_state.write_image_cache_state(&finish_ctx);
+  std::unique_lock locker(lock);
+  image_cache_state.write_image_cache_state(locker, &finish_ctx);
+  ASSERT_FALSE(locker.owns_lock());
   ASSERT_EQ(0, finish_ctx.wait());
 }