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>

diff --git a/src/librbd/cache/pwl/AbstractWriteLog.cc b/src/librbd/cache/pwl/AbstractWriteLog.cc
index 72fcf4fe95d466188f272dc7aff9f905db992ea2..580726ddffc67e7b93eeab9798649bece78ecbd5 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.cc
+++ b/src/librbd/cache/pwl/AbstractWriteLog.cc
@@ -314,11 +314,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
@@ -331,6 +327,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>
@@ -573,11 +572,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>
@@ -624,11 +623,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);
       }
@@ -659,17 +656,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
   {
@@ -1807,7 +1803,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) {
@@ -2023,21 +2020,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 fe6e1087d05205df6ea0472f51f8f0474d82f40d..ab941df0f65fc38fdee9fd68f9642637cee7b7a9 100644 (file)
--- a/src/librbd/cache/pwl/ImageCacheState.cc
+++ b/src/librbd/cache/pwl/ImageCacheState.cc
@@ -60,9 +60,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;
@@ -82,7 +83,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 1fdd5d966b08872ed07d1e5cfc281af494a45604..e922ba543aa27852f0b9498a311ac76106a133c7 100644 (file)
--- a/src/librbd/cache/pwl/rwl/WriteLog.cc
+++ b/src/librbd/cache/pwl/rwl/WriteLog.cc
@@ -100,33 +100,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 1fee2d13e72e2562d6520dbf5d4dd68285efc726..753b15b69f7f953e351f45bc5e0967966aa96a3c 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.cc
+++ b/src/librbd/cache/pwl/ssd/WriteLog.cc
@@ -531,23 +531,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);
   }
 }
 
@@ -841,7 +836,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());
 }