librbd: commit IO as safe when complete if writeback cache is disabled

We do not need to flush IO to ensure its safe if the writeback cache is
disabled when performing a journal replay. Instead, immediately mark the
IO as safe and let the journal's periodic commit throttle handle updating
the position.

Fixes: http://tracker.ceph.com/issues/23516
Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 254f3573230962df705d3980f03861252cc75162..83a92ea94642edd9b4f4e55b58a457bc0120fa43 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -904,6 +904,10 @@ public:
     journal_policy = policy;
   }
 
+  bool ImageCtx::is_writeback_cache_enabled() const {
+    return (cache && cache_max_dirty > 0);
+  }
+
   void ImageCtx::get_thread_pool_instance(CephContext *cct,
                                           ThreadPool **thread_pool,
                                           ContextWQ **op_work_queue) {

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index e0e4ec556aa5691386291801eb395d6538aa5592..1b2953b4bba0a26cc66701413afdc3561f03a223 100644 (file)
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -316,6 +316,8 @@ namespace librbd {
     journal::Policy *get_journal_policy() const;
     void set_journal_policy(journal::Policy *policy);
 
+    bool is_writeback_cache_enabled() const;
+
     static void get_thread_pool_instance(CephContext *cct,
                                          ThreadPool **thread_pool,
                                          ContextWQ **op_work_queue);

diff --git a/src/librbd/journal/Replay.cc b/src/librbd/journal/Replay.cc
index 4af7158880ea2101f13e94cbae43275257a300dd..89c2bcd858efb9ca4f738d38eb4f7eea0a2186cd 100644 (file)
--- a/src/librbd/journal/Replay.cc
+++ b/src/librbd/journal/Replay.cc
@@ -882,7 +882,8 @@ void Replay<I>::handle_event(const journal::UnknownEvent &event,
 
 template <typename I>
 void Replay<I>::handle_aio_modify_complete(Context *on_ready, Context *on_safe,
-                                           int r, std::set<int> &filters) {
+                                           int r, std::set<int> &filters,
+                                           bool writeback_cache_enabled) {
   Mutex::Locker locker(m_lock);
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 20) << ": on_ready=" << on_ready << ", "
@@ -901,8 +902,23 @@ void Replay<I>::handle_aio_modify_complete(Context *on_ready, Context *on_safe,
     return;
   }
 
-  // will be completed after next flush operation completes
-  m_aio_modify_safe_contexts.insert(on_safe);
+  if (writeback_cache_enabled) {
+    // will be completed after next flush operation completes
+    m_aio_modify_safe_contexts.insert(on_safe);
+  } else {
+    // IO is safely stored on disk
+    assert(m_in_flight_aio_modify > 0);
+    --m_in_flight_aio_modify;
+
+    if (m_on_aio_ready != nullptr) {
+      ldout(cct, 10) << ": resuming paused AIO" << dendl;
+      m_on_aio_ready->complete(0);
+      m_on_aio_ready = nullptr;
+    }
+
+    ldout(cct, 20) << ": completing safe context: " << on_safe << dendl;
+    m_image_ctx.op_work_queue->queue(on_safe, 0);
+  }
 }
 
 template <typename I>
@@ -1081,13 +1097,18 @@ Replay<I>::create_aio_modify_completion(Context *on_ready,
   }
 
   ++m_in_flight_aio_modify;
-  m_aio_modify_unsafe_contexts.push_back(on_safe);
+
+  bool writeback_cache_enabled = m_image_ctx.is_writeback_cache_enabled();
+  if (writeback_cache_enabled) {
+    m_aio_modify_unsafe_contexts.push_back(on_safe);
+  }
 
   // FLUSH if we hit the low-water mark -- on_safe contexts are
   // completed by flushes-only so that we don't move the journal
   // commit position until safely on-disk
 
-  *flush_required = (m_aio_modify_unsafe_contexts.size() ==
+  *flush_required = (writeback_cache_enabled &&
+                     m_aio_modify_unsafe_contexts.size() ==
                        IN_FLIGHT_IO_LOW_WATER_MARK);
   if (*flush_required) {
     ldout(cct, 10) << ": hit AIO replay low-water mark: scheduling flush"
@@ -1110,7 +1131,8 @@ Replay<I>::create_aio_modify_completion(Context *on_ready,
   // event. when flushed, the completion of the next flush will fire the
   // on_safe callback
   auto aio_comp = io::AioCompletion::create_and_start<Context>(
-    new C_AioModifyComplete(this, on_ready, on_safe, std::move(filters)),
+    new C_AioModifyComplete(this, on_ready, on_safe, std::move(filters),
+                            writeback_cache_enabled),
     util::get_image_ctx(&m_image_ctx), aio_type);
   return aio_comp;
 }

diff --git a/src/librbd/journal/Replay.h b/src/librbd/journal/Replay.h
index 6e058ddb35b7d0db8eafc83d569218fc4cf685ab..206d7db46bbd9c54dd9295cdc296bc47340cdc88 100644 (file)
--- a/src/librbd/journal/Replay.h
+++ b/src/librbd/journal/Replay.h
@@ -78,13 +78,17 @@ private:
     Context *on_ready;
     Context *on_safe;
     std::set<int> filters;
+    bool writeback_cache_enabled;
     C_AioModifyComplete(Replay *replay, Context *on_ready,
-                        Context *on_safe, std::set<int> &&filters)
+                        Context *on_safe, std::set<int> &&filters,
+                        bool writeback_cache_enabled)
       : replay(replay), on_ready(on_ready), on_safe(on_safe),
-        filters(std::move(filters)) {
+        filters(std::move(filters)),
+        writeback_cache_enabled(writeback_cache_enabled) {
     }
     void finish(int r) override {
-      replay->handle_aio_modify_complete(on_ready, on_safe, r, filters);
+      replay->handle_aio_modify_complete(on_ready, on_safe, r, filters,
+                                         writeback_cache_enabled);
     }
   };
 
@@ -177,7 +181,8 @@ private:
                     Context *on_safe);
 
   void handle_aio_modify_complete(Context *on_ready, Context *on_safe,
-                                  int r, std::set<int> &filters);
+                                  int r, std::set<int> &filters,
+                                  bool writeback_cache_enabled);
   void handle_aio_flush_complete(Context *on_flush_safe, Contexts &on_safe_ctxs,
                                  int r);
 

diff --git a/src/test/librbd/journal/test_mock_Replay.cc b/src/test/librbd/journal/test_mock_Replay.cc
index 95bd5c50ac833592262f96f9c08561954b9d3835..5c82b552eb179c91c4bc2ba0231d763222a61eb4 100644 (file)
--- a/src/test/librbd/journal/test_mock_Replay.cc
+++ b/src/test/librbd/journal/test_mock_Replay.cc
@@ -293,6 +293,12 @@ public:
     }
   }
 
+  void expect_writeback_cache_enabled(MockReplayImageCtx &mock_image_ctx,
+                                      bool enabled) {
+    EXPECT_CALL(mock_image_ctx, is_writeback_cache_enabled())
+      .WillRepeatedly(Return(enabled));
+  }
+
   void when_process(MockJournalReplay &mock_journal_replay,
                     EventEntry &&event_entry, Context *on_ready,
                     Context *on_safe) {
@@ -372,6 +378,7 @@ TEST_F(TestMockJournalReplay, AioDiscard) {
 
   MockJournalReplay mock_journal_replay(mock_image_ctx);
   MockIoImageRequest mock_io_image_request;
+  expect_writeback_cache_enabled(mock_image_ctx, true);
   expect_op_work_queue(mock_image_ctx);
 
   InSequence seq;
@@ -405,6 +412,7 @@ TEST_F(TestMockJournalReplay, AioWrite) {
 
   MockJournalReplay mock_journal_replay(mock_image_ctx);
   MockIoImageRequest mock_io_image_request;
+  expect_writeback_cache_enabled(mock_image_ctx, true);
   expect_op_work_queue(mock_image_ctx);
 
   InSequence seq;
@@ -469,6 +477,7 @@ TEST_F(TestMockJournalReplay, AioWriteSame) {
 
   MockJournalReplay mock_journal_replay(mock_image_ctx);
   MockIoImageRequest mock_io_image_request;
+  expect_writeback_cache_enabled(mock_image_ctx, true);
   expect_op_work_queue(mock_image_ctx);
 
   InSequence seq;
@@ -505,6 +514,7 @@ TEST_F(TestMockJournalReplay, AioCompareAndWrite) {
   MockJournalReplay mock_compare_and_write_journal_replay(mock_image_ctx);
   MockJournalReplay mock_mis_compare_and_write_journal_replay(mock_image_ctx);
   MockIoImageRequest mock_io_image_request;
+  expect_writeback_cache_enabled(mock_image_ctx, true);
   expect_op_work_queue(mock_image_ctx);
 
   InSequence seq;
@@ -565,6 +575,7 @@ TEST_F(TestMockJournalReplay, IOError) {
 
   MockJournalReplay mock_journal_replay(mock_image_ctx);
   MockIoImageRequest mock_io_image_request;
+  expect_writeback_cache_enabled(mock_image_ctx, true);
   expect_op_work_queue(mock_image_ctx);
 
   InSequence seq;
@@ -598,6 +609,7 @@ TEST_F(TestMockJournalReplay, SoftFlushIO) {
 
   MockJournalReplay mock_journal_replay(mock_image_ctx);
   MockIoImageRequest mock_io_image_request;
+  expect_writeback_cache_enabled(mock_image_ctx, true);
   expect_op_work_queue(mock_image_ctx);
 
   InSequence seq;
@@ -642,6 +654,7 @@ TEST_F(TestMockJournalReplay, PauseIO) {
 
   MockJournalReplay mock_journal_replay(mock_image_ctx);
   MockIoImageRequest mock_io_image_request;
+  expect_writeback_cache_enabled(mock_image_ctx, true);
   expect_op_work_queue(mock_image_ctx);
 
   InSequence seq;
@@ -677,6 +690,8 @@ TEST_F(TestMockJournalReplay, PauseIO) {
 }
 
 TEST_F(TestMockJournalReplay, Flush) {
+  REQUIRE_FEATURE(RBD_FEATURE_JOURNALING);
+
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
 
@@ -688,6 +703,7 @@ TEST_F(TestMockJournalReplay, Flush) {
 
   MockJournalReplay mock_journal_replay(mock_image_ctx);
   MockIoImageRequest mock_io_image_request;
+  expect_writeback_cache_enabled(mock_image_ctx, true);
   expect_op_work_queue(mock_image_ctx);
 
   InSequence seq;
@@ -790,6 +806,8 @@ TEST_F(TestMockJournalReplay, BlockedOpFinishError) {
 }
 
 TEST_F(TestMockJournalReplay, MissingOpFinishEvent) {
+  REQUIRE_FEATURE(RBD_FEATURE_JOURNALING);
+
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
 
@@ -849,6 +867,8 @@ TEST_F(TestMockJournalReplay, MissingOpFinishEvent) {
 }
 
 TEST_F(TestMockJournalReplay, MissingOpFinishEventCancelOps) {
+  REQUIRE_FEATURE(RBD_FEATURE_JOURNALING);
+
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
 
@@ -901,6 +921,8 @@ TEST_F(TestMockJournalReplay, MissingOpFinishEventCancelOps) {
 }
 
 TEST_F(TestMockJournalReplay, UnknownOpFinishEvent) {
+  REQUIRE_FEATURE(RBD_FEATURE_JOURNALING);
+
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
 
@@ -2013,5 +2035,37 @@ TEST_F(TestMockJournalReplay, LockLostBeforeExecuteOp) {
   ASSERT_EQ(-ECANCELED, on_finish_safe.wait());
 }
 
+TEST_F(TestMockJournalReplay, WritebackCacheDisabled) {
+  REQUIRE_FEATURE(RBD_FEATURE_JOURNALING);
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  MockReplayImageCtx mock_image_ctx(*ictx);
+
+  MockExclusiveLock mock_exclusive_lock;
+  mock_image_ctx.exclusive_lock = &mock_exclusive_lock;
+  expect_accept_ops(mock_exclusive_lock, true);
+
+  MockJournalReplay mock_journal_replay(mock_image_ctx);
+  MockIoImageRequest mock_io_image_request;
+  expect_writeback_cache_enabled(mock_image_ctx, false);
+  expect_op_work_queue(mock_image_ctx);
+
+  InSequence seq;
+  io::AioCompletion *aio_comp;
+  C_SaferCond on_ready;
+  C_SaferCond on_safe;
+  expect_aio_discard(mock_io_image_request, &aio_comp, 123, 456, false);
+  when_process(mock_journal_replay,
+               EventEntry{AioDiscardEvent(123, 456, false)},
+               &on_ready, &on_safe);
+
+  when_complete(mock_image_ctx, aio_comp, 0);
+  ASSERT_EQ(0, on_ready.wait());
+  ASSERT_EQ(0, on_safe.wait());
+  ASSERT_EQ(0, when_shut_down(mock_journal_replay, false));
+}
+
 } // namespace journal
 } // namespace librbd

diff --git a/src/test/librbd/mock/MockImageCtx.h b/src/test/librbd/mock/MockImageCtx.h
index 8c77ca16ffb84c73bb19beae7456a5361b93b11d..44ade806c755c0143aa5faeb5aee5db2bdaffd55 100644 (file)
--- a/src/test/librbd/mock/MockImageCtx.h
+++ b/src/test/librbd/mock/MockImageCtx.h
@@ -214,6 +214,8 @@ struct MockImageCtx {
   MOCK_CONST_METHOD0(get_stripe_count, uint64_t());
   MOCK_CONST_METHOD0(get_stripe_period, uint64_t());
 
+  MOCK_CONST_METHOD0(is_writeback_cache_enabled, bool());
+
   ImageCtx *image_ctx;
   CephContext *cct;
   PerfCounters *perfcounter;

diff --git a/src/test/rbd_mirror/test_ImageReplayer.cc b/src/test/rbd_mirror/test_ImageReplayer.cc
index 5ea29c6d5d1a03f1c6859a5e8060ad62140db047..c3ea3246b2f1be45e09cab39bc1af4c745778543 100644 (file)
--- a/src/test/rbd_mirror/test_ImageReplayer.cc
+++ b/src/test/rbd_mirror/test_ImageReplayer.cc
@@ -82,6 +82,11 @@ public:
   TestImageReplayer()
     : m_local_cluster(new librados::Rados()), m_watch_handle(0)
   {
+    EXPECT_EQ(0, g_ceph_context->_conf->get_val("rbd_mirror_journal_commit_age",
+                                                &m_journal_commit_age));
+    EXPECT_EQ(0, g_ceph_context->_conf->set_val("rbd_mirror_journal_commit_age",
+                                                "0.1"));
+
     EXPECT_EQ("", connect_cluster_pp(*m_local_cluster.get()));
     EXPECT_EQ(0, m_local_cluster->conf_set("rbd_cache", "false"));
     EXPECT_EQ(0, m_local_cluster->conf_set("rbd_mirror_journal_poll_age", "1"));
@@ -139,6 +144,8 @@ public:
 
     EXPECT_EQ(0, m_remote_cluster.pool_delete(m_remote_pool_name.c_str()));
     EXPECT_EQ(0, m_local_cluster->pool_delete(m_local_pool_name.c_str()));
+    EXPECT_EQ(0, g_ceph_context->_conf->set_val("rbd_mirror_journal_commit_age",
+                                                m_journal_commit_age));
   }
 
   template <typename ImageReplayerT = rbd::mirror::ImageReplayer<> >
@@ -298,10 +305,6 @@ public:
     cls::journal::ObjectPosition mirror_position;
 
     for (int i = 0; i < 100; i++) {
-      printf("m_replayer->flush()\n");
-      C_SaferCond cond;
-      m_replayer->flush(&cond);
-      ASSERT_EQ(0, cond.wait());
       get_commit_positions(&master_position, &mirror_position);
       if (master_position == mirror_position) {
        break;
@@ -394,6 +397,7 @@ public:
   C_WatchCtx *m_watch_ctx;
   uint64_t m_watch_handle;
   char m_test_data[TEST_IO_SIZE + 1];
+  std::string m_journal_commit_age;
 };
 
 int TestImageReplayer::_image_number;