From 471898392372ba4c404376410fb56f3af5287c80 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 20 Sep 2016 13:31:36 -0400 Subject: [PATCH] test/rbd: fix possible mock journal race conditions Fixes: http://tracker.ceph.com/issues/17317 Signed-off-by: Jason Dillaman --- src/test/librbd/test_mock_Journal.cc | 148 +++++++++++++-------------- 1 file changed, 71 insertions(+), 77 deletions(-) diff --git a/src/test/librbd/test_mock_Journal.cc b/src/test/librbd/test_mock_Journal.cc index 7c69fe8bcfdba..13199b35ff382 100644 --- a/src/test/librbd/test_mock_Journal.cc +++ b/src/test/librbd/test_mock_Journal.cc @@ -174,7 +174,6 @@ using ::testing::WithArg; using namespace std::placeholders; ACTION_P2(StartReplay, wq, ctx) { - ctx->replay_handler = arg0; wq->queue(ctx, 0); } @@ -186,7 +185,6 @@ public: typedef Journal MockJournal; typedef std::function ReplayAction; - typedef std::list ReplayActions; typedef std::list Contexts; TestMockJournal() : m_lock("lock") { @@ -200,16 +198,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); } } }; @@ -268,10 +267,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) { @@ -292,11 +293,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); @@ -426,9 +432,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); @@ -451,6 +456,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) { @@ -472,22 +479,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); @@ -583,9 +589,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); @@ -599,9 +604,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); @@ -632,16 +636,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); @@ -653,9 +656,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); @@ -686,14 +688,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); @@ -706,9 +706,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); @@ -738,9 +737,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); @@ -771,16 +769,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)); @@ -789,7 +784,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); @@ -827,7 +823,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()); @@ -855,16 +851,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; @@ -880,9 +875,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); @@ -891,24 +885,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()); -- 2.39.5