From e90e3d90eb98006114173f660311e37bb4aa8cbd Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 12 Apr 2016 15:35:08 -0400 Subject: [PATCH] librbd: delay invalidation of exclusive lock pointer Certain IO and maintenance ops code paths have an expectation that the exclusive lock pointer will be valid while in-flight. Let the exclusive lock state machine clean up the pointer after it has flushed all IO and canceled all ops. Fixes: http://tracker.ceph.com/issues/15471 Signed-off-by: Jason Dillaman --- src/librbd/ExclusiveLock.cc | 57 +++++++++++++------ src/librbd/ExclusiveLock.h | 3 +- src/librbd/image/CloseRequest.cc | 21 +++++-- src/librbd/image/RefreshRequest.cc | 12 +++- .../librbd/image/test_mock_RefreshRequest.cc | 1 + 5 files changed, 69 insertions(+), 25 deletions(-) diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc index 82d00425e0bd8..5c065fe4428a6 100644 --- a/src/librbd/ExclusiveLock.cc +++ b/src/librbd/ExclusiveLock.cc @@ -111,28 +111,31 @@ void ExclusiveLock::shut_down(Context *on_shut_down) { template void ExclusiveLock::try_lock(Context *on_tried_lock) { + int r = 0; { Mutex::Locker locker(m_lock); assert(m_image_ctx.owner_lock.is_locked()); - assert(!is_shutdown()); - - if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) { + if (is_shutdown()) { + r = -ESHUTDOWN; + } else if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) { ldout(m_image_ctx.cct, 10) << this << " " << __func__ << dendl; execute_action(ACTION_TRY_LOCK, on_tried_lock); return; } } - on_tried_lock->complete(0); + on_tried_lock->complete(r); } template void ExclusiveLock::request_lock(Context *on_locked) { + int r = 0; { Mutex::Locker locker(m_lock); assert(m_image_ctx.owner_lock.is_locked()); - assert(!is_shutdown()); - if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) { + if (is_shutdown()) { + r = -ESHUTDOWN; + } else if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) { ldout(m_image_ctx.cct, 10) << this << " " << __func__ << dendl; execute_action(ACTION_REQUEST_LOCK, on_locked); return; @@ -140,25 +143,26 @@ void ExclusiveLock::request_lock(Context *on_locked) { } if (on_locked != nullptr) { - on_locked->complete(0); + on_locked->complete(r); } } template void ExclusiveLock::release_lock(Context *on_released) { + int r = 0; { Mutex::Locker locker(m_lock); assert(m_image_ctx.owner_lock.is_locked()); - assert(!is_shutdown()); - - if (m_state != STATE_UNLOCKED || !m_actions_contexts.empty()) { + if (is_shutdown()) { + r = -ESHUTDOWN; + } else if (m_state != STATE_UNLOCKED || !m_actions_contexts.empty()) { ldout(m_image_ctx.cct, 10) << this << " " << __func__ << dendl; execute_action(ACTION_RELEASE_LOCK, on_released); return; } } - on_released->complete(0); + on_released->complete(r); } template @@ -466,10 +470,8 @@ void ExclusiveLock::send_shutdown() { assert(m_lock.is_locked()); if (m_state == STATE_UNLOCKED) { m_state = STATE_SHUTTING_DOWN; - m_image_ctx.aio_work_queue->clear_require_lock_on_read(); - m_image_ctx.aio_work_queue->unblock_writes(); - m_image_ctx.image_watcher->flush(util::create_context_callback< - ExclusiveLock, &ExclusiveLock::complete_shutdown>(this)); + m_image_ctx.op_work_queue->queue(util::create_context_callback< + ExclusiveLock, &ExclusiveLock::handle_unlocked_shutdown>(this), 0); return; } @@ -493,15 +495,20 @@ void ExclusiveLock::send_shutdown_release() { using el = ExclusiveLock; ReleaseRequest* req = ReleaseRequest::create( m_image_ctx, cookie, nullptr, - util::create_context_callback(this)); + util::create_context_callback(this)); req->send(); } template -void ExclusiveLock::handle_shutdown(int r) { +void ExclusiveLock::handle_locked_shutdown(int r) { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl; + { + RWLock::WLocker owner_locker(m_image_ctx.owner_lock); + m_image_ctx.exclusive_lock = nullptr; + } + if (r < 0) { lderr(cct) << "failed to shut down exclusive lock: " << cpp_strerror(r) << dendl; @@ -514,6 +521,22 @@ void ExclusiveLock::handle_shutdown(int r) { complete_shutdown(r); } +template +void ExclusiveLock::handle_unlocked_shutdown(int r) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl; + + { + RWLock::WLocker owner_locker(m_image_ctx.owner_lock); + m_image_ctx.exclusive_lock = nullptr; + } + + m_image_ctx.aio_work_queue->clear_require_lock_on_read(); + m_image_ctx.aio_work_queue->unblock_writes(); + m_image_ctx.image_watcher->flush(util::create_context_callback< + ExclusiveLock, &ExclusiveLock::complete_shutdown>(this)); +} + template void ExclusiveLock::complete_shutdown(int r) { ActionContexts action_contexts; diff --git a/src/librbd/ExclusiveLock.h b/src/librbd/ExclusiveLock.h index 910c67f2df59f..c547372dd1af3 100644 --- a/src/librbd/ExclusiveLock.h +++ b/src/librbd/ExclusiveLock.h @@ -151,7 +151,8 @@ private: void send_shutdown(); void send_shutdown_release(); - void handle_shutdown(int r); + void handle_locked_shutdown(int r); + void handle_unlocked_shutdown(int r); void complete_shutdown(int r); }; diff --git a/src/librbd/image/CloseRequest.cc b/src/librbd/image/CloseRequest.cc index 55e25ab37648d..b953860f5122f 100644 --- a/src/librbd/image/CloseRequest.cc +++ b/src/librbd/image/CloseRequest.cc @@ -87,9 +87,10 @@ template void CloseRequest::send_shut_down_exclusive_lock() { { RWLock::WLocker owner_locker(m_image_ctx->owner_lock); - RWLock::WLocker snap_locker(m_image_ctx->snap_lock); - std::swap(m_exclusive_lock, m_image_ctx->exclusive_lock); + m_exclusive_lock = m_image_ctx->exclusive_lock; + // if reading a snapshot -- possible object map is open + RWLock::WLocker snap_locker(m_image_ctx->snap_lock); if (m_exclusive_lock == nullptr) { delete m_image_ctx->object_map; m_image_ctx->object_map = nullptr; @@ -104,6 +105,8 @@ void CloseRequest::send_shut_down_exclusive_lock() { CephContext *cct = m_image_ctx->cct; ldout(cct, 10) << this << " " << __func__ << dendl; + // in-flight IO will be flushed and in-flight requests will be canceled + // before releasing lock m_exclusive_lock->shut_down(create_context_callback< CloseRequest, &CloseRequest::handle_shut_down_exclusive_lock>(this)); } @@ -113,10 +116,18 @@ void CloseRequest::handle_shut_down_exclusive_lock(int r) { CephContext *cct = m_image_ctx->cct; ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl; - // object map and journal closed during exclusive lock shutdown - assert(m_image_ctx->journal == nullptr); - assert(m_image_ctx->object_map == nullptr); + { + RWLock::RLocker owner_locker(m_image_ctx->owner_lock); + assert(m_image_ctx->exclusive_lock == nullptr); + + // object map and journal closed during exclusive lock shutdown + RWLock::RLocker snap_locker(m_image_ctx->snap_lock); + assert(m_image_ctx->journal == nullptr); + assert(m_image_ctx->object_map == nullptr); + } + delete m_exclusive_lock; + m_exclusive_lock = nullptr; save_result(r); if (r < 0) { diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc index 12bee84493d02..5894f5032f22c 100644 --- a/src/librbd/image/RefreshRequest.cc +++ b/src/librbd/image/RefreshRequest.cc @@ -581,7 +581,8 @@ Context *RefreshRequest::send_v2_shut_down_exclusive_lock() { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << this << " " << __func__ << dendl; - // exclusive lock feature was dynamically disabled + // exclusive lock feature was dynamically disabled. in-flight IO will be + // flushed and in-flight requests will be canceled before releasing lock using klass = RefreshRequest; Context *ctx = create_context_callback< klass, &klass::handle_v2_shut_down_exclusive_lock>(this); @@ -600,6 +601,11 @@ Context *RefreshRequest::handle_v2_shut_down_exclusive_lock(int *result) { save_result(result); } + { + RWLock::WLocker owner_locker(m_image_ctx.owner_lock); + assert(m_image_ctx.exclusive_lock == nullptr); + } + assert(m_exclusive_lock != nullptr); delete m_exclusive_lock; m_exclusive_lock = nullptr; @@ -784,9 +790,11 @@ void RefreshRequest::apply() { m_image_ctx.snap_lock)) { // disabling exclusive lock will automatically handle closing // object map and journaling - std::swap(m_exclusive_lock, m_image_ctx.exclusive_lock); + assert(m_exclusive_lock == nullptr); + m_exclusive_lock = m_image_ctx.exclusive_lock; } else { if (m_exclusive_lock != nullptr) { + assert(m_image_ctx.exclusive_lock == nullptr); std::swap(m_exclusive_lock, m_image_ctx.exclusive_lock); } if (!m_image_ctx.test_features(RBD_FEATURE_JOURNALING, diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc index e362986ac632c..e6c5912834e72 100644 --- a/src/test/librbd/image/test_mock_RefreshRequest.cc +++ b/src/test/librbd/image/test_mock_RefreshRequest.cc @@ -75,6 +75,7 @@ ACTION_P(TestFeatures, image_ctx) { ACTION_P(ShutDownExclusiveLock, image_ctx) { // shutting down exclusive lock will close object map and journal + image_ctx->exclusive_lock = nullptr; image_ctx->object_map = nullptr; image_ctx->journal = nullptr; } -- 2.39.5