librbd/cache/pwl: avoid inconsistencies in ImageCacheState

When empty and/or clean bools are updated in I/O handling code paths,
ImageCacheState becomes inconistent for a short while: e.g. with clean
transitioned to true, dirty_bytes counter could still be positive
because the counters are updated only in periodic_stats().  Move to
updating the counters in update_image_cache_state(Context*) to avoid
this.

update_image_cache_state(Context*) now requires m_lock -- most call
sites already hold it anyway.  The only problematic call site was
AbstractWriteLog::shut_down() callback chain: perf_stop() needed to
be moved to the very end since perf counters must be alive now for
update_image_cache_state() to work.

Don't override expect_op_work_queue() in unit tests: completing
context in the same thread now results in a deadlock on m_lock in
all test cases that call AbstractWriteLog::init().

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 5faf827f580ea9513d51932a75a6022181b78b04..aa18de0bcd95a48c0fd5be78a680452fdeb2d277 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.cc
+++ b/src/librbd/cache/pwl/AbstractWriteLog.cc
@@ -315,17 +315,6 @@ void AbstractWriteLog<I>::log_perf() {
 template <typename I>
 void AbstractWriteLog<I>::periodic_stats() {
   std::lock_guard locker(m_lock);
-  m_cache_state->allocated_bytes = m_bytes_allocated;
-  m_cache_state->cached_bytes = m_bytes_cached;
-  m_cache_state->dirty_bytes = m_bytes_dirty;
-  m_cache_state->free_bytes = m_bytes_allocated_cap - m_bytes_allocated;
-  m_cache_state->hits_full = m_perfcounter->get(l_librbd_pwl_rd_hit_req);
-  m_cache_state->hits_partial = m_perfcounter->get(l_librbd_pwl_rd_part_hit_req);
-  m_cache_state->misses = m_perfcounter->get(l_librbd_pwl_rd_req) -
-      m_cache_state->hits_full - m_cache_state->hits_partial;
-  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;
   update_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()
@@ -591,6 +580,19 @@ void AbstractWriteLog<I>::update_image_cache_state() {
 template <typename I>
 void AbstractWriteLog<I>::update_image_cache_state(Context *on_finish) {
   ldout(m_image_ctx.cct, 10) << dendl;
+
+  ceph_assert(ceph_mutex_is_locked_by_me(m_lock));
+  m_cache_state->allocated_bytes = m_bytes_allocated;
+  m_cache_state->cached_bytes = m_bytes_cached;
+  m_cache_state->dirty_bytes = m_bytes_dirty;
+  m_cache_state->free_bytes = m_bytes_allocated_cap - m_bytes_allocated;
+  m_cache_state->hits_full = m_perfcounter->get(l_librbd_pwl_rd_hit_req);
+  m_cache_state->hits_partial = m_perfcounter->get(l_librbd_pwl_rd_part_hit_req);
+  m_cache_state->misses = m_perfcounter->get(l_librbd_pwl_rd_req) -
+      m_cache_state->hits_full - m_cache_state->hits_partial;
+  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);
 }
 
@@ -620,6 +622,7 @@ 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);
       } else {
         on_finish->complete(r);
@@ -639,6 +642,9 @@ void AbstractWriteLog<I>::shut_down(Context *on_finish) {
 
   Context *ctx = new LambdaContext(
     [this, on_finish](int r) {
+      if (m_perfcounter) {
+        perf_stop();
+      }
       ldout(m_image_ctx.cct, 6) << "shutdown complete" << dendl;
       m_image_ctx.op_work_queue->queue(on_finish, r);
     });
@@ -647,20 +653,14 @@ void AbstractWriteLog<I>::shut_down(Context *on_finish) {
       ldout(m_image_ctx.cct, 6) << "image cache cleaned" << dendl;
       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();
 
-        if (m_perfcounter) {
-          perf_stop();
-        }
-      }
+      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);
     });
   ctx = new LambdaContext(

diff --git a/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc b/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc
index d4b1079b5c2655655d37fd22932d1d3ba1c1827a..fc372b0d8c7a16a40df83428cb750eef92580a1d 100644 (file)
--- a/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc
+++ b/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc
@@ -84,13 +84,6 @@ struct TestMockCacheReplicatedWriteLog : public TestMockFixture {
     ASSERT_EQ(size, state.size);
   }
 
-  void expect_op_work_queue(MockImageCtx& mock_image_ctx) {
-    EXPECT_CALL(*mock_image_ctx.op_work_queue, queue(_, _))
-      .WillRepeatedly(Invoke([](Context* ctx, int r) {
-                        ctx->complete(r);
-                      }));
-  }
-
   void expect_context_complete(MockContextRWL& mock_context, int r) {
     EXPECT_CALL(mock_context, complete(r))
       .WillRepeatedly(Invoke([&mock_context](int r) {

diff --git a/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc b/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc
index 81a248b8e8c54366c7008d37bbef531951150a2f..b1fc4726461a5396516262d4edb83591fb637781 100644 (file)
--- a/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc
+++ b/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc
@@ -86,13 +86,6 @@ struct TestMockCacheSSDWriteLog : public TestMockFixture {
     ASSERT_EQ(size, state.size);
   }
 
-  void expect_op_work_queue(MockImageCtx& mock_image_ctx) {
-    EXPECT_CALL(*mock_image_ctx.op_work_queue, queue(_, _))
-      .WillRepeatedly(Invoke([](Context* ctx, int r) {
-                        ctx->complete(r);
-                      }));
-  }
-
   void expect_context_complete(MockContextSSD& mock_context, int r) {
     EXPECT_CALL(mock_context, complete(r))
       .WillRepeatedly(Invoke([&mock_context](int r) {