From 4a525671b3541a0a208dd039ac96f42bc8fca2cc Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Thu, 22 Jun 2017 09:10:20 -0400 Subject: [PATCH] librbd: directly inform IO work queue when locks are required Due to lock dependency issues between librbd locks and the the thread pool lock, it's impossible to directly determine if the lock is required within the _void_dequeue method. Therefore, this state needs to be internally tracked by the work queue. Signed-off-by: Jason Dillaman --- src/librbd/ExclusiveLock.cc | 16 ++-- .../exclusive_lock/PreReleaseRequest.cc | 7 +- src/librbd/image/RefreshRequest.cc | 13 ++-- src/librbd/io/ImageRequestWQ.cc | 36 +++++---- src/librbd/io/ImageRequestWQ.h | 5 +- src/librbd/io/Types.h | 6 ++ .../test_mock_PreReleaseRequest.cc | 14 +++- .../librbd/image/test_mock_RefreshRequest.cc | 73 ++----------------- src/test/librbd/mock/io/MockImageRequestWQ.h | 5 +- src/test/librbd/test_mock_ExclusiveLock.cc | 19 ++--- 10 files changed, 82 insertions(+), 112 deletions(-) diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc index a3d69fce89f..f126ccbb482 100644 --- a/src/librbd/ExclusiveLock.cc +++ b/src/librbd/ExclusiveLock.cc @@ -147,8 +147,14 @@ template void ExclusiveLock::handle_init_complete(uint64_t features) { ldout(m_image_ctx.cct, 10) << ": features=" << features << dendl; - if ((features & RBD_FEATURE_JOURNALING) != 0) { - m_image_ctx.io_work_queue->set_require_lock_on_read(); + { + RWLock::RLocker owner_locker(m_image_ctx.owner_lock); + if (m_image_ctx.clone_copy_on_read || + (features & RBD_FEATURE_JOURNALING) != 0) { + m_image_ctx.io_work_queue->set_require_lock(io::DIRECTION_BOTH, true); + } else { + m_image_ctx.io_work_queue->set_require_lock(io::DIRECTION_WRITE, true); + } } Mutex::Locker locker(ML::m_lock); @@ -161,7 +167,7 @@ void ExclusiveLock::shutdown_handler(int r, Context *on_finish) { { RWLock::WLocker owner_locker(m_image_ctx.owner_lock); - m_image_ctx.io_work_queue->clear_require_lock_on_read(); + m_image_ctx.io_work_queue->set_require_lock(io::DIRECTION_BOTH, false); m_image_ctx.exclusive_lock = nullptr; } @@ -269,7 +275,7 @@ void ExclusiveLock::handle_post_acquired_lock(int r) { if (r >= 0) { m_image_ctx.image_watcher->notify_acquired_lock(); - m_image_ctx.io_work_queue->clear_require_lock_on_read(); + m_image_ctx.io_work_queue->set_require_lock(io::DIRECTION_BOTH, false); m_image_ctx.io_work_queue->unblock_writes(); } @@ -311,7 +317,7 @@ void ExclusiveLock::post_release_lock_handler(bool shutting_down, int r, } else { { RWLock::WLocker owner_locker(m_image_ctx.owner_lock); - m_image_ctx.io_work_queue->clear_require_lock_on_read(); + m_image_ctx.io_work_queue->set_require_lock(io::DIRECTION_BOTH, false); m_image_ctx.exclusive_lock = nullptr; } diff --git a/src/librbd/exclusive_lock/PreReleaseRequest.cc b/src/librbd/exclusive_lock/PreReleaseRequest.cc index 5a37acc96a5..1fd6e2bfd22 100644 --- a/src/librbd/exclusive_lock/PreReleaseRequest.cc +++ b/src/librbd/exclusive_lock/PreReleaseRequest.cc @@ -109,8 +109,11 @@ void PreReleaseRequest::send_block_writes() { { RWLock::RLocker owner_locker(m_image_ctx.owner_lock); - if (m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) { - m_image_ctx.io_work_queue->set_require_lock_on_read(); + if (m_image_ctx.clone_copy_on_read || + m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) { + m_image_ctx.io_work_queue->set_require_lock(io::DIRECTION_BOTH, true); + } else { + m_image_ctx.io_work_queue->set_require_lock(io::DIRECTION_WRITE, true); } m_image_ctx.io_work_queue->block_writes(ctx); } diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc index 7397b681a71..f8448dc0e01 100644 --- a/src/librbd/image/RefreshRequest.cc +++ b/src/librbd/image/RefreshRequest.cc @@ -644,7 +644,8 @@ void RefreshRequest::send_v2_open_journal() { !journal_disabled_by_policy && m_image_ctx.exclusive_lock != nullptr && m_image_ctx.journal == nullptr) { - m_image_ctx.io_work_queue->set_require_lock_on_read(); + m_image_ctx.io_work_queue->set_require_lock(librbd::io::DIRECTION_BOTH, + true); } send_v2_block_writes(); return; @@ -1077,7 +1078,6 @@ void RefreshRequest::apply() { // object map and journaling assert(m_exclusive_lock == nullptr); m_exclusive_lock = m_image_ctx.exclusive_lock; - m_image_ctx.io_work_queue->clear_require_lock_on_read(); } else { if (m_exclusive_lock != nullptr) { assert(m_image_ctx.exclusive_lock == nullptr); @@ -1085,8 +1085,9 @@ void RefreshRequest::apply() { } if (!m_image_ctx.test_features(RBD_FEATURE_JOURNALING, m_image_ctx.snap_lock)) { - if (m_image_ctx.journal != nullptr) { - m_image_ctx.io_work_queue->clear_require_lock_on_read(); + if (!m_image_ctx.clone_copy_on_read && m_image_ctx.journal != nullptr) { + m_image_ctx.io_work_queue->set_require_lock(io::DIRECTION_READ, + false); } std::swap(m_journal, m_image_ctx.journal); } else if (m_journal != nullptr) { @@ -1097,10 +1098,6 @@ void RefreshRequest::apply() { m_object_map != nullptr) { std::swap(m_object_map, m_image_ctx.object_map); } - if (m_image_ctx.clone_copy_on_read && - m_image_ctx.io_work_queue->is_lock_required()) { - m_image_ctx.io_work_queue->set_require_lock_on_read(); - } } } } diff --git a/src/librbd/io/ImageRequestWQ.cc b/src/librbd/io/ImageRequestWQ.cc index d80606af515..577aa3801fe 100644 --- a/src/librbd/io/ImageRequestWQ.cc +++ b/src/librbd/io/ImageRequestWQ.cc @@ -392,28 +392,36 @@ void ImageRequestWQ::unblock_writes() { } template -void ImageRequestWQ::set_require_lock_on_read() { - CephContext *cct = m_image_ctx.cct; - ldout(cct, 20) << dendl; - - RWLock::WLocker locker(m_lock); - m_require_lock_on_read = true; -} - -template -void ImageRequestWQ::clear_require_lock_on_read() { +void ImageRequestWQ::set_require_lock(Direction direction, bool enabled) { CephContext *cct = m_image_ctx.cct; ldout(cct, 20) << dendl; + bool wake_up = false; { RWLock::WLocker locker(m_lock); - if (!m_require_lock_on_read) { - return; + switch (direction) { + case DIRECTION_READ: + wake_up = (enabled != m_require_lock_on_read); + m_require_lock_on_read = enabled; + break; + case DIRECTION_WRITE: + wake_up = (enabled != m_require_lock_on_write); + m_require_lock_on_write = enabled; + break; + case DIRECTION_BOTH: + wake_up = (enabled != m_require_lock_on_read || + enabled != m_require_lock_on_write); + m_require_lock_on_read = enabled; + m_require_lock_on_write = enabled; + break; } + } - m_require_lock_on_read = false; + // wake up the thread pool whenever the state changes so that + // we can re-request the lock if required + if (wake_up) { + this->signal(); } - this->signal(); } template diff --git a/src/librbd/io/ImageRequestWQ.h b/src/librbd/io/ImageRequestWQ.h index 6abfbdd7d99..b3a91c3010b 100644 --- a/src/librbd/io/ImageRequestWQ.h +++ b/src/librbd/io/ImageRequestWQ.h @@ -7,6 +7,7 @@ #include "include/Context.h" #include "common/RWLock.h" #include "common/WorkQueue.h" +#include "librbd/io/Types.h" #include #include @@ -62,8 +63,7 @@ public: void block_writes(Context *on_blocked); void unblock_writes(); - void set_require_lock_on_read(); - void clear_require_lock_on_read(); + void set_require_lock(Direction direction, bool enabled); protected: void *_void_dequeue() override; @@ -101,6 +101,7 @@ private: Contexts m_write_blocker_contexts; uint32_t m_write_blockers = 0; bool m_require_lock_on_read = false; + bool m_require_lock_on_write = false; std::atomic m_queued_reads { 0 }; std::atomic m_queued_writes { 0 }; std::atomic m_in_flight_ios { 0 }; diff --git a/src/librbd/io/Types.h b/src/librbd/io/Types.h index 874e1dd3e28..006b2229842 100644 --- a/src/librbd/io/Types.h +++ b/src/librbd/io/Types.h @@ -23,6 +23,12 @@ typedef enum { AIO_TYPE_WRITESAME, } aio_type_t; +enum Direction { + DIRECTION_READ, + DIRECTION_WRITE, + DIRECTION_BOTH +}; + typedef std::vector > Extents; typedef std::map ExtentMap; diff --git a/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc b/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc index 99de2551212..44aa012568d 100644 --- a/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc +++ b/src/test/librbd/exclusive_lock/test_mock_PreReleaseRequest.cc @@ -53,15 +53,21 @@ public: .WillOnce(Return(enabled)); } - void expect_set_require_lock_on_read(MockImageCtx &mock_image_ctx) { - EXPECT_CALL(*mock_image_ctx.io_work_queue, set_require_lock_on_read()); + void expect_set_require_lock(MockImageCtx &mock_image_ctx, + librbd::io::Direction direction, bool enabled) { + EXPECT_CALL(*mock_image_ctx.io_work_queue, set_require_lock(direction, + enabled)); } void expect_block_writes(MockImageCtx &mock_image_ctx, int r) { expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, ((mock_image_ctx.features & RBD_FEATURE_JOURNALING) != 0)); - if ((mock_image_ctx.features & RBD_FEATURE_JOURNALING) != 0) { - expect_set_require_lock_on_read(mock_image_ctx); + if (mock_image_ctx.clone_copy_on_read || + (mock_image_ctx.features & RBD_FEATURE_JOURNALING) != 0) { + expect_set_require_lock(mock_image_ctx, librbd::io::DIRECTION_BOTH, true); + } else { + expect_set_require_lock(mock_image_ctx, librbd::io::DIRECTION_WRITE, + true); } EXPECT_CALL(*mock_image_ctx.io_work_queue, block_writes(_)) .WillOnce(CompleteContext(r, mock_image_ctx.image_ctx->op_work_queue)); diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc index 858e98e197c..5c50d8131a6 100644 --- a/src/test/librbd/image/test_mock_RefreshRequest.cc +++ b/src/test/librbd/image/test_mock_RefreshRequest.cc @@ -97,16 +97,10 @@ public: typedef RefreshRequest MockRefreshRequest; typedef RefreshParentRequest MockRefreshParentRequest; - void expect_is_lock_required(MockRefreshImageCtx &mock_image_ctx, bool require_lock) { - EXPECT_CALL(*mock_image_ctx.io_work_queue, is_lock_required()).WillOnce(Return(require_lock)); - } - - void expect_set_require_lock_on_read(MockRefreshImageCtx &mock_image_ctx) { - EXPECT_CALL(*mock_image_ctx.io_work_queue, set_require_lock_on_read()); - } - - void expect_clear_require_lock_on_read(MockRefreshImageCtx &mock_image_ctx) { - EXPECT_CALL(*mock_image_ctx.io_work_queue, clear_require_lock_on_read()); + void expect_set_require_lock(MockRefreshImageCtx &mock_image_ctx, + librbd::io::Direction direction, bool enabled) { + EXPECT_CALL(*mock_image_ctx.io_work_queue, set_require_lock(direction, + enabled)); } void expect_v1_read_header(MockRefreshImageCtx &mock_image_ctx, int r) { @@ -637,7 +631,6 @@ TEST_F(TestMockImageRefreshRequest, DisableExclusiveLock) { expect_get_flags(mock_image_ctx, 0); expect_get_group(mock_image_ctx, 0); expect_refresh_parent_is_required(mock_refresh_parent_request, false); - expect_clear_require_lock_on_read(mock_image_ctx); expect_shut_down_exclusive_lock(mock_image_ctx, *mock_exclusive_lock, 0); C_SaferCond ctx; @@ -742,58 +735,6 @@ TEST_F(TestMockImageRefreshRequest, JournalDisabledByPolicy) { ASSERT_EQ(0, ctx.wait()); } -TEST_F(TestMockImageRefreshRequest, ExclusiveLockWithCoR) { - REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); - - std::string val; - ASSERT_EQ(0, _rados.conf_get("rbd_clone_copy_on_read", val)); - if (val == "false") { - std::cout << "SKIPPING due to disabled rbd_copy_on_read" << std::endl; - return; - } - - librbd::ImageCtx *ictx; - ASSERT_EQ(0, open_image(m_image_name, &ictx)); - - MockRefreshImageCtx mock_image_ctx(*ictx); - MockRefreshParentRequest mock_refresh_parent_request; - - MockExclusiveLock mock_exclusive_lock; - mock_image_ctx.exclusive_lock = &mock_exclusive_lock; - - if (ictx->test_features(RBD_FEATURE_JOURNALING)) { - ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_JOURNALING, - false)); - } - - if (ictx->test_features(RBD_FEATURE_FAST_DIFF)) { - ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_FAST_DIFF, - false)); - } - - if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) { - ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_OBJECT_MAP, - false)); - } - - expect_op_work_queue(mock_image_ctx); - expect_test_features(mock_image_ctx); - - InSequence seq; - expect_get_mutable_metadata(mock_image_ctx, 0); - expect_get_flags(mock_image_ctx, 0); - expect_get_group(mock_image_ctx, 0); - expect_refresh_parent_is_required(mock_refresh_parent_request, false); - expect_is_lock_required(mock_image_ctx, true); - expect_set_require_lock_on_read(mock_image_ctx); - - C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, false, &ctx); - req->send(); - - ASSERT_EQ(0, ctx.wait()); -} - TEST_F(TestMockImageRefreshRequest, EnableJournalWithExclusiveLock) { REQUIRE_FEATURE(RBD_FEATURE_JOURNALING); @@ -873,7 +814,7 @@ TEST_F(TestMockImageRefreshRequest, EnableJournalWithoutExclusiveLock) { expect_get_flags(mock_image_ctx, 0); expect_get_group(mock_image_ctx, 0); expect_refresh_parent_is_required(mock_refresh_parent_request, false); - expect_set_require_lock_on_read(mock_image_ctx); + expect_set_require_lock(mock_image_ctx, librbd::io::DIRECTION_BOTH, true); C_SaferCond ctx; MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, false, &ctx); @@ -917,7 +858,9 @@ TEST_F(TestMockImageRefreshRequest, DisableJournal) { expect_get_group(mock_image_ctx, 0); expect_refresh_parent_is_required(mock_refresh_parent_request, false); expect_block_writes(mock_image_ctx, 0); - expect_clear_require_lock_on_read(mock_image_ctx); + if (!mock_image_ctx.clone_copy_on_read) { + expect_set_require_lock(mock_image_ctx, librbd::io::DIRECTION_READ, false); + } expect_close_journal(mock_image_ctx, *mock_journal, 0); expect_unblock_writes(mock_image_ctx); diff --git a/src/test/librbd/mock/io/MockImageRequestWQ.h b/src/test/librbd/mock/io/MockImageRequestWQ.h index 049d1bc233e..c3187e086cf 100644 --- a/src/test/librbd/mock/io/MockImageRequestWQ.h +++ b/src/test/librbd/mock/io/MockImageRequestWQ.h @@ -5,6 +5,7 @@ #define CEPH_TEST_LIBRBD_MOCK_IO_IMAGE_REQUEST_WQ_H #include "gmock/gmock.h" +#include "librbd/io/Types.h" class Context; @@ -15,10 +16,8 @@ struct MockImageRequestWQ { MOCK_METHOD1(block_writes, void(Context *)); MOCK_METHOD0(unblock_writes, void()); - MOCK_METHOD0(set_require_lock_on_read, void()); - MOCK_METHOD0(clear_require_lock_on_read, void()); + MOCK_METHOD2(set_require_lock, void(Direction, bool)); - MOCK_CONST_METHOD0(is_lock_required, bool()); MOCK_CONST_METHOD0(is_lock_request_needed, bool()); }; diff --git a/src/test/librbd/test_mock_ExclusiveLock.cc b/src/test/librbd/test_mock_ExclusiveLock.cc index 2a3388bdb39..6496af5089b 100644 --- a/src/test/librbd/test_mock_ExclusiveLock.cc +++ b/src/test/librbd/test_mock_ExclusiveLock.cc @@ -214,24 +214,25 @@ public: .WillOnce(Return(ret_val)); } - void expect_set_require_lock_on_read(MockExclusiveLockImageCtx &mock_image_ctx) { - EXPECT_CALL(*mock_image_ctx.io_work_queue, set_require_lock_on_read()); - } - - void expect_clear_require_lock_on_read(MockExclusiveLockImageCtx &mock_image_ctx) { - EXPECT_CALL(*mock_image_ctx.io_work_queue, clear_require_lock_on_read()); + void expect_set_require_lock(MockExclusiveLockImageCtx &mock_image_ctx, + io::Direction direction, bool enabled) { + EXPECT_CALL(*mock_image_ctx.io_work_queue, set_require_lock(direction, + enabled)); } void expect_block_writes(MockExclusiveLockImageCtx &mock_image_ctx) { EXPECT_CALL(*mock_image_ctx.io_work_queue, block_writes(_)) .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue)); - if ((mock_image_ctx.features & RBD_FEATURE_JOURNALING) != 0) { - expect_set_require_lock_on_read(mock_image_ctx); + if (mock_image_ctx.clone_copy_on_read || + (mock_image_ctx.features & RBD_FEATURE_JOURNALING) != 0) { + expect_set_require_lock(mock_image_ctx, io::DIRECTION_BOTH, true); + } else { + expect_set_require_lock(mock_image_ctx, io::DIRECTION_WRITE, true); } } void expect_unblock_writes(MockExclusiveLockImageCtx &mock_image_ctx) { - expect_clear_require_lock_on_read(mock_image_ctx); + expect_set_require_lock(mock_image_ctx, io::DIRECTION_BOTH, false); EXPECT_CALL(*mock_image_ctx.io_work_queue, unblock_writes()); } -- 2.47.3