From 8451c0f86eaf79395f0502ed235e4932522cec86 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 12 Apr 2016 16:17:05 -0400 Subject: [PATCH] librbd: cancel ops before blocking IO Ops might have in-flight IO -- blocking IO after canceling the ops will result in the in-flight IO being flushed. Shutdown also requires an intermediate state where is still acts like it downs the lock until after all ops are canceled and all IO is flushed. Signed-off-by: Jason Dillaman --- src/librbd/ExclusiveLock.cc | 27 +++++++++--- src/librbd/ExclusiveLock.h | 8 ++-- src/librbd/exclusive_lock/ReleaseRequest.cc | 44 +++++++++---------- src/librbd/exclusive_lock/ReleaseRequest.h | 10 ++--- .../test_mock_ReleaseRequest.cc | 5 ++- 5 files changed, 56 insertions(+), 38 deletions(-) diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc index 5c065fe4428a..fab631c6bccc 100644 --- a/src/librbd/ExclusiveLock.cc +++ b/src/librbd/ExclusiveLock.cc @@ -61,6 +61,7 @@ bool ExclusiveLock::is_lock_owner() const { case STATE_LOCKED: case STATE_POST_ACQUIRING: case STATE_PRE_RELEASING: + case STATE_PRE_SHUTTING_DOWN: lock_owner = true; break; default: @@ -214,6 +215,7 @@ bool ExclusiveLock::is_transition_state() const { case STATE_POST_ACQUIRING: case STATE_PRE_RELEASING: case STATE_RELEASING: + case STATE_PRE_SHUTTING_DOWN: case STATE_SHUTTING_DOWN: return true; case STATE_UNINITIALIZED: @@ -471,13 +473,13 @@ void ExclusiveLock::send_shutdown() { if (m_state == STATE_UNLOCKED) { m_state = STATE_SHUTTING_DOWN; m_image_ctx.op_work_queue->queue(util::create_context_callback< - ExclusiveLock, &ExclusiveLock::handle_unlocked_shutdown>(this), 0); + ExclusiveLock, &ExclusiveLock::handle_shutdown>(this), 0); return; } ldout(m_image_ctx.cct, 10) << this << " " << __func__ << dendl; assert(m_state == STATE_LOCKED); - m_state = STATE_SHUTTING_DOWN; + m_state = STATE_PRE_SHUTTING_DOWN; m_lock.Unlock(); m_image_ctx.op_work_queue->queue(new C_ShutDownRelease(this), 0); @@ -494,13 +496,26 @@ void ExclusiveLock::send_shutdown_release() { using el = ExclusiveLock; ReleaseRequest* req = ReleaseRequest::create( - m_image_ctx, cookie, nullptr, - util::create_context_callback(this)); + m_image_ctx, cookie, + util::create_context_callback(this), + util::create_context_callback(this)); req->send(); } template -void ExclusiveLock::handle_locked_shutdown(int r) { +void ExclusiveLock::handle_shutdown_releasing(int r) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl; + + assert(r == 0); + assert(m_state == STATE_PRE_SHUTTING_DOWN); + + // all IO and ops should be blocked/canceled by this point + m_state = STATE_SHUTTING_DOWN; +} + +template +void ExclusiveLock::handle_shutdown_released(int r) { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl; @@ -522,7 +537,7 @@ void ExclusiveLock::handle_locked_shutdown(int r) { } template -void ExclusiveLock::handle_unlocked_shutdown(int r) { +void ExclusiveLock::handle_shutdown(int r) { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl; diff --git a/src/librbd/ExclusiveLock.h b/src/librbd/ExclusiveLock.h index c547372dd1af..e2e14160a9ca 100644 --- a/src/librbd/ExclusiveLock.h +++ b/src/librbd/ExclusiveLock.h @@ -67,7 +67,7 @@ private: * | * | * v - * SHUTTING_DOWN ---> SHUTDOWN ---> + * PRE_SHUTTING_DOWN ---> SHUTTING_DOWN ---> SHUTDOWN ---> */ enum State { STATE_UNINITIALIZED, @@ -79,6 +79,7 @@ private: STATE_WAITING_FOR_PEER, STATE_PRE_RELEASING, STATE_RELEASING, + STATE_PRE_SHUTTING_DOWN, STATE_SHUTTING_DOWN, STATE_SHUTDOWN, }; @@ -151,8 +152,9 @@ private: void send_shutdown(); void send_shutdown_release(); - void handle_locked_shutdown(int r); - void handle_unlocked_shutdown(int r); + void handle_shutdown_releasing(int r); + void handle_shutdown_released(int r); + void handle_shutdown(int r); void complete_shutdown(int r); }; diff --git a/src/librbd/exclusive_lock/ReleaseRequest.cc b/src/librbd/exclusive_lock/ReleaseRequest.cc index 356beb1d12b0..0583c266d038 100644 --- a/src/librbd/exclusive_lock/ReleaseRequest.cc +++ b/src/librbd/exclusive_lock/ReleaseRequest.cc @@ -50,58 +50,58 @@ ReleaseRequest::~ReleaseRequest() { template void ReleaseRequest::send() { - send_block_writes(); + send_cancel_op_requests(); } template -void ReleaseRequest::send_block_writes() { +void ReleaseRequest::send_cancel_op_requests() { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << __func__ << dendl; using klass = ReleaseRequest; Context *ctx = create_context_callback< - klass, &klass::handle_block_writes>(this); - - { - RWLock::RLocker owner_locker(m_image_ctx.owner_lock); - if (m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) { - m_image_ctx.aio_work_queue->set_require_lock_on_read(); - } - m_image_ctx.aio_work_queue->block_writes(ctx); - } + klass, &klass::handle_cancel_op_requests>(this); + m_image_ctx.cancel_async_requests(ctx); } template -Context *ReleaseRequest::handle_block_writes(int *ret_val) { +Context *ReleaseRequest::handle_cancel_op_requests(int *ret_val) { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl; - if (*ret_val < 0) { - m_image_ctx.aio_work_queue->unblock_writes(); - return m_on_finish; - } + assert(*ret_val == 0); - send_cancel_op_requests(); + send_block_writes(); return nullptr; } template -void ReleaseRequest::send_cancel_op_requests() { +void ReleaseRequest::send_block_writes() { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << __func__ << dendl; using klass = ReleaseRequest; Context *ctx = create_context_callback< - klass, &klass::handle_cancel_op_requests>(this); - m_image_ctx.cancel_async_requests(ctx); + klass, &klass::handle_block_writes>(this); + + { + RWLock::RLocker owner_locker(m_image_ctx.owner_lock); + if (m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) { + m_image_ctx.aio_work_queue->set_require_lock_on_read(); + } + m_image_ctx.aio_work_queue->block_writes(ctx); + } } template -Context *ReleaseRequest::handle_cancel_op_requests(int *ret_val) { +Context *ReleaseRequest::handle_block_writes(int *ret_val) { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl; - assert(*ret_val == 0); + if (*ret_val < 0) { + m_image_ctx.aio_work_queue->unblock_writes(); + return m_on_finish; + } if (m_on_releasing != nullptr) { // alert caller that we no longer own the exclusive lock diff --git a/src/librbd/exclusive_lock/ReleaseRequest.h b/src/librbd/exclusive_lock/ReleaseRequest.h index c5bf91cca99f..8712bc963cc8 100644 --- a/src/librbd/exclusive_lock/ReleaseRequest.h +++ b/src/librbd/exclusive_lock/ReleaseRequest.h @@ -33,10 +33,10 @@ private: * * | * v - * BLOCK_WRITES + * CANCEL_OP_REQUESTS * | * v - * CANCEL_OP_REQUESTS + * BLOCK_WRITES * | * v * FLUSH_NOTIFIES . . . . . . . . . . . . . . @@ -67,12 +67,12 @@ private: decltype(m_image_ctx.object_map) m_object_map; decltype(m_image_ctx.journal) m_journal; - void send_block_writes(); - Context *handle_block_writes(int *ret_val); - void send_cancel_op_requests(); Context *handle_cancel_op_requests(int *ret_val); + void send_block_writes(); + Context *handle_block_writes(int *ret_val); + void send_flush_notifies(); Context *handle_flush_notifies(int *ret_val); diff --git a/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc b/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc index eb42feb715ff..2ed8b8ef9a54 100644 --- a/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc +++ b/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc @@ -93,8 +93,8 @@ TEST_F(TestMockExclusiveLockReleaseRequest, Success) { expect_op_work_queue(mock_image_ctx); InSequence seq; - expect_block_writes(mock_image_ctx, 0); expect_cancel_op_requests(mock_image_ctx, 0); + expect_block_writes(mock_image_ctx, 0); expect_flush_notifies(mock_image_ctx); MockJournal *mock_journal = new MockJournal(); @@ -183,6 +183,7 @@ TEST_F(TestMockExclusiveLockReleaseRequest, BlockWritesError) { expect_op_work_queue(mock_image_ctx); InSequence seq; + expect_cancel_op_requests(mock_image_ctx, 0); expect_block_writes(mock_image_ctx, -EINVAL); expect_unblock_writes(mock_image_ctx); @@ -204,8 +205,8 @@ TEST_F(TestMockExclusiveLockReleaseRequest, UnlockError) { expect_op_work_queue(mock_image_ctx); InSequence seq; - expect_block_writes(mock_image_ctx, 0); expect_cancel_op_requests(mock_image_ctx, 0); + expect_block_writes(mock_image_ctx, 0); expect_flush_notifies(mock_image_ctx); expect_unlock(mock_image_ctx, -EINVAL); -- 2.47.3