test/rbd: fix possible mock journal race conditions

Fixes: http://tracker.ceph.com/issues/17317
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 471898392372ba4c404376410fb56f3af5287c80)

diff --git a/src/test/librbd/test_mock_Journal.cc b/src/test/librbd/test_mock_Journal.cc
index 3785f2ce783e2a4e7fe208774f6925a9bd676e22..3864cdbfec211719d611d54900bfbed3aa6736e5 100644 (file)
--- a/src/test/librbd/test_mock_Journal.cc
+++ b/src/test/librbd/test_mock_Journal.cc
@@ -109,7 +109,6 @@ using ::testing::WithArg;
 using namespace std::placeholders;
 
 ACTION_P2(StartReplay, wq, ctx) {
-  ctx->replay_handler = arg0;
   wq->queue(ctx, 0);
 }
 
@@ -121,7 +120,6 @@ public:
   typedef Journal<MockJournalImageCtx> MockJournal;
 
   typedef std::function<void(::journal::ReplayHandler*)> ReplayAction;
-  typedef std::list<ReplayAction> ReplayActions;
   typedef std::list<Context *> Contexts;
 
   TestMockJournal() : m_lock("lock") {
@@ -135,16 +133,17 @@ public:
   Cond m_cond;
   Contexts m_commit_contexts;
 
-  struct C_StartReplay : public Context {
-    ReplayActions replay_actions;
-    ::journal::ReplayHandler *replay_handler;
+  struct C_ReplayAction : public Context {
+    ::journal::ReplayHandler **replay_handler;
+    ReplayAction replay_action;
 
-    C_StartReplay(const ReplayActions &replay_actions)
-      : replay_actions(replay_actions), replay_handler(nullptr) {
+    C_ReplayAction(::journal::ReplayHandler **replay_handler,
+                   const ReplayAction &replay_action)
+      : replay_handler(replay_handler), replay_action(replay_action) {
     }
     virtual void finish(int r) {
-      for (auto &action : replay_actions) {
-        action(replay_handler);
+      if (replay_action) {
+        replay_action(*replay_handler);
       }
     }
   };
@@ -203,10 +202,12 @@ public:
 
   void expect_start_replay(MockJournalImageCtx &mock_image_ctx,
                            ::journal::MockJournaler &mock_journaler,
-                           const ReplayActions &actions) {
+                           const ReplayAction &action) {
     EXPECT_CALL(mock_journaler, start_replay(_))
-                 .WillOnce(StartReplay(mock_image_ctx.image_ctx->op_work_queue,
-                                       new C_StartReplay(actions)));
+                 .WillOnce(DoAll(SaveArg<0>(&m_replay_handler),
+                           StartReplay(mock_image_ctx.image_ctx->op_work_queue,
+                                       new C_ReplayAction(&m_replay_handler,
+                                                          action))));
   }
 
   void expect_stop_replay(::journal::MockJournaler &mock_journaler) {
@@ -227,11 +228,16 @@ public:
                   .WillOnce(Return(bufferlist()));
   }
 
-  void expect_try_pop_front(::journal::MockJournaler &mock_journaler,
+  void expect_try_pop_front(MockJournalImageCtx &mock_image_ctx,
+                            ::journal::MockJournaler &mock_journaler,
                             bool entries_available,
-                            ::journal::MockReplayEntry &mock_replay_entry) {
+                            ::journal::MockReplayEntry &mock_replay_entry,
+                            const ReplayAction &action = {}) {
     EXPECT_CALL(mock_journaler, try_pop_front(_))
                   .WillOnce(DoAll(SetArgPointee<0>(::journal::MockReplayEntryProxy()),
+                                  StartReplay(mock_image_ctx.image_ctx->op_work_queue,
+                                              new C_ReplayAction(&m_replay_handler,
+                                                                 action)),
                                   Return(entries_available)));
     if (entries_available) {
       expect_get_data(mock_replay_entry);
@@ -361,9 +367,8 @@ public:
     expect_get_journaler_cached_client(mock_journaler, 0);
     expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
     expect_start_replay(
-      mock_image_ctx, mock_journaler, {
-        std::bind(&invoke_replay_complete, _1, 0)
-      });
+      mock_image_ctx, mock_journaler,
+      std::bind(&invoke_replay_complete, _1, 0));
 
     MockJournalReplay mock_journal_replay;
     expect_stop_replay(mock_journaler);
@@ -386,6 +391,8 @@ public:
   static void invoke_replay_complete(::journal::ReplayHandler *handler, int r) {
     handler->handle_complete(r);
   }
+
+  ::journal::ReplayHandler *m_replay_handler = nullptr;
 };
 
 TEST_F(TestMockJournal, StateTransitions) {
@@ -407,22 +414,21 @@ TEST_F(TestMockJournal, StateTransitions) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_ready, _1),
-      std::bind(&invoke_replay_ready, _1),
-      std::bind(&invoke_replay_complete, _1, 0)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_ready, _1));
 
   ::journal::MockReplayEntry mock_replay_entry;
   MockJournalReplay mock_journal_replay;
-  expect_try_pop_front(mock_journaler, true, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, true, mock_replay_entry);
   expect_replay_process(mock_journal_replay);
-  expect_try_pop_front(mock_journaler, true, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, true, mock_replay_entry);
   expect_replay_process(mock_journal_replay);
-  expect_try_pop_front(mock_journaler, false, mock_replay_entry);
-  expect_try_pop_front(mock_journaler, true, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, false, mock_replay_entry,
+                       std::bind(&invoke_replay_ready, _1));
+  expect_try_pop_front(mock_image_ctx, mock_journaler, true, mock_replay_entry);
   expect_replay_process(mock_journal_replay);
-  expect_try_pop_front(mock_journaler, false, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, false, mock_replay_entry,
+                       std::bind(&invoke_replay_complete, _1, 0));
 
   expect_stop_replay(mock_journaler);
   expect_shut_down_replay(mock_image_ctx, mock_journal_replay, 0);
@@ -518,9 +524,8 @@ TEST_F(TestMockJournal, ReplayCompleteError) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_complete, _1, -EINVAL)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_complete, _1, -EINVAL));
 
   MockJournalReplay mock_journal_replay;
   expect_stop_replay(mock_journaler);
@@ -534,9 +539,8 @@ TEST_F(TestMockJournal, ReplayCompleteError) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_complete, _1, 0)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_complete, _1, 0));
 
   expect_stop_replay(mock_journaler);
   expect_shut_down_replay(mock_image_ctx, mock_journal_replay, 0);
@@ -567,16 +571,15 @@ TEST_F(TestMockJournal, FlushReplayError) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_ready, _1),
-      std::bind(&invoke_replay_complete, _1, 0)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_ready, _1));
 
   ::journal::MockReplayEntry mock_replay_entry;
   MockJournalReplay mock_journal_replay;
-  expect_try_pop_front(mock_journaler, true, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, true, mock_replay_entry);
   expect_replay_process(mock_journal_replay);
-  expect_try_pop_front(mock_journaler, false, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, false, mock_replay_entry,
+                       std::bind(&invoke_replay_complete, _1, 0));
   expect_stop_replay(mock_journaler);
   expect_shut_down_replay(mock_image_ctx, mock_journal_replay, -EINVAL);
   expect_shut_down_journaler(mock_journaler);
@@ -588,9 +591,8 @@ TEST_F(TestMockJournal, FlushReplayError) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_complete, _1, 0)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_complete, _1, 0));
 
   expect_stop_replay(mock_journaler);
   expect_shut_down_replay(mock_image_ctx, mock_journal_replay, 0);
@@ -621,14 +623,12 @@ TEST_F(TestMockJournal, CorruptEntry) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_ready, _1),
-      std::bind(&invoke_replay_complete, _1, 0)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_ready, _1));
 
   ::journal::MockReplayEntry mock_replay_entry;
   MockJournalReplay mock_journal_replay;
-  expect_try_pop_front(mock_journaler, true, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, true, mock_replay_entry);
   EXPECT_CALL(mock_journal_replay, decode(_, _)).WillOnce(Return(-EBADMSG));
   expect_stop_replay(mock_journaler);
   expect_shut_down_replay(mock_image_ctx, mock_journal_replay, 0, true);
@@ -641,9 +641,8 @@ TEST_F(TestMockJournal, CorruptEntry) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_complete, _1, 0)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_complete, _1, 0));
   expect_stop_replay(mock_journaler);
   expect_shut_down_replay(mock_image_ctx, mock_journal_replay, 0);
   expect_start_append(mock_journaler);
@@ -673,9 +672,8 @@ TEST_F(TestMockJournal, StopError) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_complete, _1, 0)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_complete, _1, 0));
 
   MockJournalReplay mock_journal_replay;
   expect_stop_replay(mock_journaler);
@@ -706,16 +704,13 @@ TEST_F(TestMockJournal, ReplayOnDiskPreFlushError) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
 
-  ::journal::ReplayHandler *replay_handler = nullptr;
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_ready, _1),
-      [&replay_handler] (::journal::ReplayHandler *handler) {replay_handler = handler;},
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_ready, _1));
 
   ::journal::MockReplayEntry mock_replay_entry;
   MockJournalReplay mock_journal_replay;
-  expect_try_pop_front(mock_journaler, true, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, true, mock_replay_entry);
 
   EXPECT_CALL(mock_journal_replay, decode(_, _))
                 .WillOnce(Return(0));
@@ -724,7 +719,8 @@ TEST_F(TestMockJournal, ReplayOnDiskPreFlushError) {
                 .WillOnce(DoAll(SaveArg<1>(&on_ready),
                                 WithArg<2>(Invoke(this, &TestMockJournal::save_commit_context))));
 
-  expect_try_pop_front(mock_journaler, false, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, false,
+                       mock_replay_entry);
   expect_stop_replay(mock_journaler);
   expect_shut_down_replay(mock_image_ctx, mock_journal_replay, 0, true);
   expect_shut_down_journaler(mock_journaler);
@@ -762,7 +758,7 @@ TEST_F(TestMockJournal, ReplayOnDiskPreFlushError) {
   on_safe->complete(-EINVAL);
 
   // flag the replay as complete
-  replay_handler->handle_complete(0);
+  m_replay_handler->handle_complete(0);
 
   ASSERT_EQ(0, ctx.wait());
 
@@ -790,16 +786,15 @@ TEST_F(TestMockJournal, ReplayOnDiskPostFlushError) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_ready, _1),
-      std::bind(&invoke_replay_complete, _1, 0)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_ready, _1));
 
   ::journal::MockReplayEntry mock_replay_entry;
   MockJournalReplay mock_journal_replay;
-  expect_try_pop_front(mock_journaler, true, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, true, mock_replay_entry);
   expect_replay_process(mock_journal_replay);
-  expect_try_pop_front(mock_journaler, false, mock_replay_entry);
+  expect_try_pop_front(mock_image_ctx, mock_journaler, false, mock_replay_entry,
+                       std::bind(&invoke_replay_complete, _1, 0));
   expect_stop_replay(mock_journaler);
 
   Context *on_flush = nullptr;
@@ -815,9 +810,8 @@ TEST_F(TestMockJournal, ReplayOnDiskPostFlushError) {
   expect_get_journaler_cached_client(mock_journaler, 0);
   expect_get_journaler_tags(mock_image_ctx, mock_journaler, 0);
   expect_start_replay(
-    mock_image_ctx, mock_journaler, {
-      std::bind(&invoke_replay_complete, _1, 0)
-    });
+    mock_image_ctx, mock_journaler,
+    std::bind(&invoke_replay_complete, _1, 0));
 
   expect_stop_replay(mock_journaler);
   expect_shut_down_replay(mock_image_ctx, mock_journal_replay, 0);
@@ -826,24 +820,24 @@ TEST_F(TestMockJournal, ReplayOnDiskPostFlushError) {
   C_SaferCond ctx;
   mock_journal.open(&ctx);
 
+  // proceed with the flush
   {
-    // wait for the on_safe process callback
+    // wait for on_flush callback
     Mutex::Locker locker(m_lock);
-    while (m_commit_contexts.empty()) {
+    while (on_flush == nullptr) {
       m_cond.Wait(m_lock);
     }
   }
-  m_commit_contexts.front()->complete(-EINVAL);
-  m_commit_contexts.clear();
 
-  // proceed with the flush
   {
-    // wait for on_flush callback
+    // wait for the on_safe process callback
     Mutex::Locker locker(m_lock);
-    while (on_flush == nullptr) {
+    while (m_commit_contexts.empty()) {
       m_cond.Wait(m_lock);
     }
   }
+  m_commit_contexts.front()->complete(-EINVAL);
+  m_commit_contexts.clear();
   on_flush->complete(0);
 
   ASSERT_EQ(0, ctx.wait());