From 58ed9a4076b3b5b0a37d726d49e16059b73ee239 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 18 Jan 2017 11:14:56 -0500 Subject: [PATCH] librbd: clean up pre-release lock handling Signed-off-by: Jason Dillaman --- src/librbd/ExclusiveLock.cc | 28 ++----------------- src/librbd/ExclusiveLock.h | 4 +-- src/librbd/ManagedLock.cc | 13 ++++++--- src/librbd/ManagedLock.h | 10 ------- .../exclusive_lock/PreReleaseRequest.cc | 21 ++++---------- src/librbd/exclusive_lock/PreReleaseRequest.h | 9 +++--- .../test_mock_PreReleaseRequest.cc | 19 ++++--------- src/test/librbd/test_mock_ExclusiveLock.cc | 10 ++++--- 8 files changed, 35 insertions(+), 79 deletions(-) diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc index cd6684a042760a..dfb67fbf9a8ecf 100644 --- a/src/librbd/ExclusiveLock.cc +++ b/src/librbd/ExclusiveLock.cc @@ -30,8 +30,7 @@ ExclusiveLock::ExclusiveLock(I &image_ctx) image_ctx.image_watcher, managed_lock::EXCLUSIVE, image_ctx.blacklist_on_break_lock, image_ctx.blacklist_expire_seconds), - m_image_ctx(image_ctx), m_pre_post_callback(nullptr), - m_shutting_down(false) { + m_image_ctx(image_ctx) { Mutex::Locker locker(ML::m_lock); ML::set_state_uninitialized(); } @@ -249,34 +248,13 @@ void ExclusiveLock::pre_release_lock_handler(bool shutting_down, ldout(m_image_ctx.cct, 10) << dendl; Mutex::Locker locker(ML::m_lock); - m_shutting_down = shutting_down; - - using EL = ExclusiveLock; - PreReleaseRequest *req = PreReleaseRequest::create(m_image_ctx, - util::create_context_callback(this), - on_finish, shutting_down); - + PreReleaseRequest *req = PreReleaseRequest::create( + m_image_ctx, shutting_down, on_finish); m_image_ctx.op_work_queue->queue(new FunctionContext([req](int r) { req->send(); })); } -template -void ExclusiveLock::handle_pre_releasing_lock(int r) { - ldout(m_image_ctx.cct, 10) << dendl; - - Mutex::Locker locker(ML::m_lock); - - assert(r == 0); - - // all IO and ops should be blocked/canceled by this point - if (!m_shutting_down) { - ML::set_state_releasing(); - } else { - ML::set_state_shutting_down(); - } -} - template void ExclusiveLock::post_release_lock_handler(bool shutting_down, int r, Context *on_finish) { diff --git a/src/librbd/ExclusiveLock.h b/src/librbd/ExclusiveLock.h index c12949c0fb9a11..c03cf6414d2db4 100644 --- a/src/librbd/ExclusiveLock.h +++ b/src/librbd/ExclusiveLock.h @@ -92,8 +92,7 @@ class ExclusiveLock : public ManagedLock { }; ImageCtxT& m_image_ctx; - Context *m_pre_post_callback; - bool m_shutting_down; + Context *m_pre_post_callback = nullptr; uint32_t m_request_blocked_count = 0; int m_request_blocked_ret_val = 0; @@ -103,7 +102,6 @@ class ExclusiveLock : public ManagedLock { void handle_init_complete(); void handle_post_acquiring_lock(int r); void handle_post_acquired_lock(int r); - void handle_pre_releasing_lock(int r); }; } // namespace librbd diff --git a/src/librbd/ManagedLock.cc b/src/librbd/ManagedLock.cc index 5673694b01629d..21062bce454a66 100644 --- a/src/librbd/ManagedLock.cc +++ b/src/librbd/ManagedLock.cc @@ -240,10 +240,6 @@ void ManagedLock::post_acquire_lock_handler(int r, Context *on_finish) { template void ManagedLock::pre_release_lock_handler(bool shutting_down, Context *on_finish) { - { - Mutex::Locker locker(m_lock); - m_state = shutting_down ? STATE_SHUTTING_DOWN : STATE_RELEASING; - } on_finish->complete(0); } @@ -565,6 +561,12 @@ template void ManagedLock::handle_pre_release_lock(int r) { ldout(m_cct, 10) << ": r=" << r << dendl; + { + Mutex::Locker locker(m_lock); + assert(m_state == STATE_PRE_RELEASING); + m_state = STATE_RELEASING; + } + if (r < 0) { handle_release_lock(r); return; @@ -654,6 +656,9 @@ void ManagedLock::handle_shutdown_pre_release(int r) { { Mutex::Locker locker(m_lock); cookie = m_cookie; + + assert(m_state == STATE_PRE_SHUTTING_DOWN); + m_state = STATE_SHUTTING_DOWN; } using managed_lock::ReleaseRequest; diff --git a/src/librbd/ManagedLock.h b/src/librbd/ManagedLock.h index 7cbc295a93665a..b8bec661b918e8 100644 --- a/src/librbd/ManagedLock.h +++ b/src/librbd/ManagedLock.h @@ -89,16 +89,6 @@ class ManagedLock { assert(m_state == STATE_ACQUIRING); m_state = STATE_POST_ACQUIRING; } - inline void set_state_releasing() { - assert(m_lock.is_locked()); - assert(m_state == STATE_PRE_RELEASING); - m_state = STATE_RELEASING; - } - inline void set_state_shutting_down() { - assert(m_lock.is_locked()); - assert(m_state == STATE_PRE_SHUTTING_DOWN); - m_state = STATE_SHUTTING_DOWN; - } bool is_state_shutdown() const; inline bool is_state_acquiring() const { diff --git a/src/librbd/exclusive_lock/PreReleaseRequest.cc b/src/librbd/exclusive_lock/PreReleaseRequest.cc index 5096ed8189c14b..44f9821720114b 100644 --- a/src/librbd/exclusive_lock/PreReleaseRequest.cc +++ b/src/librbd/exclusive_lock/PreReleaseRequest.cc @@ -24,17 +24,15 @@ using util::create_context_callback; template PreReleaseRequest* PreReleaseRequest::create(I &image_ctx, - Context *on_releasing, - Context *on_finish, - bool shutting_down) { - return new PreReleaseRequest(image_ctx, on_releasing, on_finish, - shutting_down); + bool shutting_down, + Context *on_finish) { + return new PreReleaseRequest(image_ctx, shutting_down, on_finish); } template -PreReleaseRequest::PreReleaseRequest(I &image_ctx, Context *on_releasing, - Context *on_finish, bool shutting_down) - : m_image_ctx(image_ctx), m_on_releasing(on_releasing), +PreReleaseRequest::PreReleaseRequest(I &image_ctx, bool shutting_down, + Context *on_finish) + : m_image_ctx(image_ctx), m_on_finish(create_async_context_callback(image_ctx, on_finish)), m_shutting_down(shutting_down), m_error_result(0), m_object_map(nullptr), m_journal(nullptr) { @@ -45,7 +43,6 @@ PreReleaseRequest::~PreReleaseRequest() { if (!m_shutting_down) { m_image_ctx.state->handle_prepare_lock_complete(); } - delete m_on_releasing; } template @@ -273,12 +270,6 @@ void PreReleaseRequest::send_unlock() { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << __func__ << dendl; - if (m_on_releasing != nullptr) { - // alert caller that we no longer own the exclusive lock - m_on_releasing->complete(0); - m_on_releasing = nullptr; - } - finish(); } diff --git a/src/librbd/exclusive_lock/PreReleaseRequest.h b/src/librbd/exclusive_lock/PreReleaseRequest.h index 0fda36a091ad86..98e3a1e9dac73c 100644 --- a/src/librbd/exclusive_lock/PreReleaseRequest.h +++ b/src/librbd/exclusive_lock/PreReleaseRequest.h @@ -18,8 +18,8 @@ namespace exclusive_lock { template class PreReleaseRequest { public: - static PreReleaseRequest* create(ImageCtxT &image_ctx, Context *on_releasing, - Context *on_finish, bool shutting_down); + static PreReleaseRequest* create(ImageCtxT &image_ctx, bool shutting_down, + Context *on_finish); ~PreReleaseRequest(); void send(); @@ -57,11 +57,10 @@ class PreReleaseRequest { * @endverbatim */ - PreReleaseRequest(ImageCtxT &image_ctx, Context *on_releasing, - Context *on_finish, bool shutting_down); + PreReleaseRequest(ImageCtxT &image_ctx, bool shutting_down, + Context *on_finish); ImageCtxT &m_image_ctx; - Context *m_on_releasing; Context *m_on_finish; bool m_shutting_down; diff --git a/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc b/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc index b94ab914042eb1..a2fda18bf20fb1 100644 --- a/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc +++ b/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc @@ -145,13 +145,11 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, Success) { mock_image_ctx.object_map = mock_object_map; expect_close_object_map(mock_image_ctx, *mock_object_map); - MockContext mock_releasing_ctx; - expect_complete_context(mock_releasing_ctx, 0); expect_handle_prepare_lock_complete(mock_image_ctx); C_SaferCond ctx; MockPreReleaseRequest *req = MockPreReleaseRequest::create( - mock_image_ctx, &mock_releasing_ctx, &ctx, false); + mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -179,12 +177,10 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, SuccessJournalDisabled) { expect_handle_prepare_lock_complete(mock_image_ctx); - C_SaferCond release_ctx; C_SaferCond ctx; MockPreReleaseRequest *req = MockPreReleaseRequest::create( - mock_image_ctx, &release_ctx, &ctx, false); + mock_image_ctx, false, &ctx); req->send(); - ASSERT_EQ(0, release_ctx.wait()); ASSERT_EQ(0, ctx.wait()); } @@ -207,9 +203,8 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, SuccessObjectMapDisabled) { C_SaferCond release_ctx; C_SaferCond ctx; MockPreReleaseRequest *req = MockPreReleaseRequest::create( - mock_image_ctx, &release_ctx, &ctx, true); + mock_image_ctx, true, &ctx); req->send(); - ASSERT_EQ(0, release_ctx.wait()); ASSERT_EQ(0, ctx.wait()); } @@ -240,13 +235,11 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, Blacklisted) { mock_image_ctx.object_map = mock_object_map; expect_close_object_map(mock_image_ctx, *mock_object_map); - MockContext mock_releasing_ctx; - expect_complete_context(mock_releasing_ctx, 0); expect_handle_prepare_lock_complete(mock_image_ctx); C_SaferCond ctx; MockPreReleaseRequest *req = MockPreReleaseRequest::create( - mock_image_ctx, &mock_releasing_ctx, &ctx, false); + mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -268,7 +261,7 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, BlockWritesError) { C_SaferCond ctx; MockPreReleaseRequest *req = MockPreReleaseRequest::create( - mock_image_ctx, nullptr, &ctx, true); + mock_image_ctx, true, &ctx); req->send(); ASSERT_EQ(-EINVAL, ctx.wait()); } @@ -291,7 +284,7 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, UnlockError) { C_SaferCond ctx; MockPreReleaseRequest *req = MockPreReleaseRequest::create( - mock_image_ctx, nullptr, &ctx, true); + mock_image_ctx, true, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } diff --git a/src/test/librbd/test_mock_ExclusiveLock.cc b/src/test/librbd/test_mock_ExclusiveLock.cc index 342cf57b66d977..5a97d6bc5ff8c3 100644 --- a/src/test/librbd/test_mock_ExclusiveLock.cc +++ b/src/test/librbd/test_mock_ExclusiveLock.cc @@ -63,8 +63,6 @@ struct ManagedLock { MOCK_METHOD0(set_state_unlocked, void()); MOCK_METHOD0(set_state_waiting_for_lock, void()); MOCK_METHOD0(set_state_post_acquiring, void()); - MOCK_METHOD0(set_state_releasing, void()); - MOCK_METHOD0(set_state_shutting_down, void()); MOCK_CONST_METHOD0(is_state_shutdown, bool()); MOCK_CONST_METHOD0(is_state_acquiring, bool()); @@ -90,8 +88,7 @@ struct BaseRequest { Context *on_finish = nullptr; static T* create(MockExclusiveLockImageCtx &image_ctx, - Context *on_lock_unlock, Context *on_finish, - bool shutting_down = false) { + Context *on_lock_unlock, Context *on_finish) { assert(!s_requests.empty()); T* req = s_requests.front(); req->on_lock_unlock = on_lock_unlock; @@ -124,6 +121,11 @@ struct PostAcquireRequest : public BaseRequest struct PreReleaseRequest : public BaseRequest > { + static PreReleaseRequest *create( + MockExclusiveLockImageCtx &image_ctx, bool shutting_down, + Context *on_finish) { + return BaseRequest::create(image_ctx, nullptr, on_finish); + } MOCK_METHOD0(send, void()); };