From a320a2b82174904bcc0ac45c0e95f21e2f237271 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Thu, 17 Sep 2020 16:22:20 -0400 Subject: [PATCH] librbd: reorder exclusive-lock pre-release state steps The exclusive-lock dispatch layer should be locked and flushed to ensure no IO is waiting for a refresh. Once that is complete, interlock with the refresh state machine and re-flush one last time w/ the refresh dispatch layer skipped. Signed-off-by: Jason Dillaman --- .../exclusive_lock/PreReleaseRequest.cc | 102 +++++++++++------- src/librbd/exclusive_lock/PreReleaseRequest.h | 24 +++-- .../test_mock_PreReleaseRequest.cc | 66 +++++++----- 3 files changed, 119 insertions(+), 73 deletions(-) diff --git a/src/librbd/exclusive_lock/PreReleaseRequest.cc b/src/librbd/exclusive_lock/PreReleaseRequest.cc index 644aa57415e..aadfb96e002 100644 --- a/src/librbd/exclusive_lock/PreReleaseRequest.cc +++ b/src/librbd/exclusive_lock/PreReleaseRequest.cc @@ -13,8 +13,11 @@ #include "librbd/ObjectMap.h" #include "librbd/Utils.h" #include "librbd/exclusive_lock/ImageDispatch.h" +#include "librbd/io/AioCompletion.h" +#include "librbd/io/ImageDispatchSpec.h" #include "librbd/io/ImageDispatcherInterface.h" #include "librbd/io/ObjectDispatcherInterface.h" +#include "librbd/io/Types.h" #define dout_subsys ceph_subsys_rbd #undef dout_prefix @@ -55,30 +58,6 @@ PreReleaseRequest::~PreReleaseRequest() { template void PreReleaseRequest::send() { - send_prepare_lock(); -} - -template -void PreReleaseRequest::send_prepare_lock() { - if (m_shutting_down) { - send_cancel_op_requests(); - return; - } - - CephContext *cct = m_image_ctx.cct; - ldout(cct, 10) << dendl; - - // release the lock if the image is not busy performing other actions - Context *ctx = create_context_callback< - PreReleaseRequest, &PreReleaseRequest::handle_prepare_lock>(this); - m_image_ctx.state->prepare_lock(ctx); -} - -template -void PreReleaseRequest::handle_prepare_lock(int r) { - CephContext *cct = m_image_ctx.cct; - ldout(cct, 10) << "r=" << r << dendl; - send_cancel_op_requests(); } @@ -100,17 +79,17 @@ void PreReleaseRequest::handle_cancel_op_requests(int r) { ceph_assert(r == 0); - send_block_writes(); + send_set_require_lock(); } template -void PreReleaseRequest::send_block_writes() { +void PreReleaseRequest::send_set_require_lock() { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << dendl; using klass = PreReleaseRequest; Context *ctx = create_context_callback< - klass, &klass::handle_block_writes>(this); + klass, &klass::handle_set_require_lock>(this); // setting the lock as required will automatically cause the IO // queue to re-request the lock if any IO is queued @@ -125,20 +104,13 @@ void PreReleaseRequest::send_block_writes() { } template -void PreReleaseRequest::handle_block_writes(int r) { +void PreReleaseRequest::handle_set_require_lock(int r) { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << "r=" << r << dendl; - if (r == -EBLOCKLISTED) { - // allow clean shut down if blocklisted - lderr(cct) << "failed to block writes because client is blocklisted" - << dendl; - } else if (r < 0) { - lderr(cct) << "failed to block writes: " << cpp_strerror(r) << dendl; - m_image_dispatch->unset_require_lock(io::DIRECTION_BOTH); - save_result(r); - finish(); - return; + if (r < 0) { + // IOs are still flushed regardless of the error + lderr(cct) << "failed to set lock: " << cpp_strerror(r) << dendl; } send_wait_for_ops(); @@ -159,6 +131,30 @@ void PreReleaseRequest::handle_wait_for_ops(int r) { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << dendl; + send_prepare_lock(); +} + +template +void PreReleaseRequest::send_prepare_lock() { + if (m_shutting_down) { + send_shut_down_image_cache(); + return; + } + + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << dendl; + + // release the lock if the image is not busy performing other actions + Context *ctx = create_context_callback< + PreReleaseRequest, &PreReleaseRequest::handle_prepare_lock>(this); + m_image_ctx.state->prepare_lock(ctx); +} + +template +void PreReleaseRequest::handle_prepare_lock(int r) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << "r=" << r << dendl; + send_shut_down_image_cache(); } @@ -224,6 +220,36 @@ void PreReleaseRequest::handle_invalidate_cache(int r) { return; } + send_flush_io(); +} + +template +void PreReleaseRequest::send_flush_io() { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << dendl; + + // ensure that all in-flight IO is flushed -- skipping the refresh layer + // since it should have been flushed when the lock was required and now + // refreshes are disabled / interlocked w/ this state machine. + auto ctx = create_context_callback< + PreReleaseRequest, &PreReleaseRequest::handle_flush_io>(this); + auto aio_comp = io::AioCompletion::create_and_start( + ctx, util::get_image_ctx(&m_image_ctx), librbd::io::AIO_TYPE_FLUSH); + auto req = io::ImageDispatchSpec::create_flush( + m_image_ctx, io::IMAGE_DISPATCH_LAYER_EXCLUSIVE_LOCK, aio_comp, + io::FLUSH_SOURCE_EXCLUSIVE_LOCK_SKIP_REFRESH, {}); + req->send(); +} + +template +void PreReleaseRequest::handle_flush_io(int r) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << "r=" << r << dendl; + + if (r < 0) { + lderr(cct) << "failed to flush IO: " << cpp_strerror(r) << dendl; + } + send_flush_notifies(); } diff --git a/src/librbd/exclusive_lock/PreReleaseRequest.h b/src/librbd/exclusive_lock/PreReleaseRequest.h index 6063683e2d6..8156df3415f 100644 --- a/src/librbd/exclusive_lock/PreReleaseRequest.h +++ b/src/librbd/exclusive_lock/PreReleaseRequest.h @@ -37,24 +37,27 @@ private: * * | * v - * PREPARE_LOCK - * | - * v * CANCEL_OP_REQUESTS * | * v - * BLOCK_WRITES + * SET_REQUIRE_LOCK * | * v * WAIT_FOR_OPS * | * v + * PREPARE_LOCK + * | + * v * SHUT_DOWN_IMAGE_CACHE * | * v * INVALIDATE_CACHE * | * v + * FLUSH_IO + * | + * v * FLUSH_NOTIFIES . . . . . . . . . . . . . . * | . * v . @@ -85,24 +88,27 @@ private: decltype(m_image_ctx.object_map) m_object_map = nullptr; decltype(m_image_ctx.journal) m_journal = nullptr; - void send_prepare_lock(); - void handle_prepare_lock(int r); - void send_cancel_op_requests(); void handle_cancel_op_requests(int r); - void send_block_writes(); - void handle_block_writes(int r); + void send_set_require_lock(); + void handle_set_require_lock(int r); void send_wait_for_ops(); void handle_wait_for_ops(int r); + void send_prepare_lock(); + void handle_prepare_lock(int r); + void send_shut_down_image_cache(); void handle_shut_down_image_cache(int r); void send_invalidate_cache(); void handle_invalidate_cache(int r); + void send_flush_io(); + void handle_flush_io(int r); + void send_flush_notifies(); void handle_flush_notifies(int r); diff --git a/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc b/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc index a1429c619f1..ee94ba81e56 100644 --- a/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc +++ b/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc @@ -64,6 +64,14 @@ ShutdownRequest *ShutdownRequestimage_ctx; +} + +} // namespace util } // namespace librbd // template definitions @@ -179,6 +187,26 @@ public: EXPECT_CALL(*mock_image_ctx.state, handle_prepare_lock_complete()); } + void expect_flush_io(MockTestImageCtx &mock_image_ctx, int r) { + EXPECT_CALL(*mock_image_ctx.io_image_dispatcher, send(_)) + .WillOnce(Invoke([&mock_image_ctx, r](io::ImageDispatchSpec* spec) { + ASSERT_TRUE(boost::get( + &spec->request) != nullptr); + spec->dispatch_result = io::DISPATCH_RESULT_COMPLETE; + auto aio_comp = spec->aio_comp; + auto ctx = new LambdaContext([aio_comp](int r) { + if (r < 0) { + aio_comp->fail(r); + } else { + aio_comp->set_request_count(1); + aio_comp->add_request(); + aio_comp->complete_request(r); + } + }); + mock_image_ctx.image_ctx->op_work_queue->queue(ctx, r); + })); + } + AsyncOpTracker m_async_op_tracker; }; @@ -193,11 +221,12 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, Success) { InSequence seq; - expect_prepare_lock(mock_image_ctx); expect_cancel_op_requests(mock_image_ctx, 0); MockImageDispatch mock_image_dispatch; expect_set_require_lock(mock_image_ctx, mock_image_dispatch, false, 0); + expect_prepare_lock(mock_image_ctx); + cache::MockImageCache mock_image_cache; mock_image_ctx.image_cache = &mock_image_cache; MockShutdownRequest mock_shutdown_request; @@ -205,6 +234,8 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, Success) { expect_invalidate_cache(mock_image_ctx, 0); + expect_flush_io(mock_image_ctx, 0); + expect_flush_notifies(mock_image_ctx); MockJournal mock_journal; @@ -237,8 +268,8 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, SuccessJournalDisabled) { expect_op_work_queue(mock_image_ctx); InSequence seq; - expect_prepare_lock(mock_image_ctx); expect_cancel_op_requests(mock_image_ctx, 0); + expect_prepare_lock(mock_image_ctx); cache::MockImageCache mock_image_cache; mock_image_ctx.image_cache = &mock_image_cache; @@ -247,6 +278,8 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, SuccessJournalDisabled) { expect_invalidate_cache(mock_image_ctx, 0); + expect_flush_io(mock_image_ctx, 0); + expect_flush_notifies(mock_image_ctx); MockObjectMap mock_object_map; @@ -284,6 +317,8 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, SuccessObjectMapDisabled) { expect_invalidate_cache(mock_image_ctx, 0); + expect_flush_io(mock_image_ctx, 0); + expect_flush_notifies(mock_image_ctx); C_SaferCond release_ctx; @@ -304,11 +339,11 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, Blocklisted) { expect_op_work_queue(mock_image_ctx); InSequence seq; - expect_prepare_lock(mock_image_ctx); expect_cancel_op_requests(mock_image_ctx, 0); MockImageDispatch mock_image_dispatch; expect_set_require_lock(mock_image_ctx, mock_image_dispatch, false, -EBLOCKLISTED); + expect_prepare_lock(mock_image_ctx); cache::MockImageCache mock_image_cache; mock_image_ctx.image_cache = &mock_image_cache; @@ -317,6 +352,8 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, Blocklisted) { expect_invalidate_cache(mock_image_ctx, -EBLOCKLISTED); + expect_flush_io(mock_image_ctx, -EBLOCKLISTED); + expect_flush_notifies(mock_image_ctx); MockJournal mock_journal; @@ -336,28 +373,5 @@ TEST_F(TestMockExclusiveLockPreReleaseRequest, Blocklisted) { ASSERT_EQ(0, ctx.wait()); } -TEST_F(TestMockExclusiveLockPreReleaseRequest, BlockWritesError) { - REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); - - librbd::ImageCtx *ictx; - ASSERT_EQ(0, open_image(m_image_name, &ictx)); - - MockTestImageCtx mock_image_ctx(*ictx); - - expect_op_work_queue(mock_image_ctx); - - InSequence seq; - expect_cancel_op_requests(mock_image_ctx, 0); - MockImageDispatch mock_image_dispatch; - expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true, -EINVAL); - expect_unset_require_lock(mock_image_dispatch); - - C_SaferCond ctx; - MockPreReleaseRequest *req = MockPreReleaseRequest::create( - mock_image_ctx, &mock_image_dispatch, true, m_async_op_tracker, &ctx); - req->send(); - ASSERT_EQ(-EINVAL, ctx.wait()); -} - } // namespace exclusive_lock } // namespace librbd -- 2.39.5