From: Jason Dillaman Date: Thu, 31 Aug 2017 11:51:37 +0000 (-0400) Subject: librbd: exclusive lock failures should bubble up to IO X-Git-Tag: v10.2.10~36^2~7 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=fbfafb75c70a1455e102bf09623e1f1d8cf2074f;p=ceph.git librbd: exclusive lock failures should bubble up to IO (derived from commit 048d475127b600b6a40bd5e0c3a0daf8133294f4) Signed-off-by: Jason Dillaman --- diff --git a/src/librbd/AioImageRequestWQ.cc b/src/librbd/AioImageRequestWQ.cc index 93bbe6ba37dc..f9942ce20596 100644 --- a/src/librbd/AioImageRequestWQ.cc +++ b/src/librbd/AioImageRequestWQ.cc @@ -18,6 +18,46 @@ namespace librbd { +template +struct AioImageRequestWQ::C_AcquireLock : public Context { + AioImageRequestWQ *work_queue; + AioImageRequest *image_request; + + C_AcquireLock(AioImageRequestWQ *work_queue, AioImageRequest *image_request) + : work_queue(work_queue), image_request(image_request) { + } + + void finish(int r) override { + work_queue->handle_acquire_lock(r, image_request); + } +}; + +template +struct AioImageRequestWQ::C_BlockedWrites : public Context { + AioImageRequestWQ *work_queue; + C_BlockedWrites(AioImageRequestWQ *_work_queue) + : work_queue(_work_queue) { + } + + void finish(int r) override { + work_queue->handle_blocked_writes(r); + } +}; + +template +struct AioImageRequestWQ::C_RefreshFinish : public Context { + AioImageRequestWQ *work_queue; + AioImageRequest *image_request; + + C_RefreshFinish(AioImageRequestWQ *work_queue, + AioImageRequest *image_request) + : work_queue(work_queue), image_request(image_request) { + } + void finish(int r) override { + work_queue->handle_refreshed(r, image_request); + } +}; + template AioImageRequestWQ::AioImageRequestWQ(I *image_ctx, const string &name, time_t ti, ThreadPool *tp) @@ -114,18 +154,11 @@ void AioImageRequestWQ::aio_read(AioCompletion *c, uint64_t off, uint64_t len return; } - RWLock::RLocker owner_locker(m_image_ctx.owner_lock); - // if journaling is enabled -- we need to replay the journal because // it might contain an uncommitted write - bool lock_required; - { - RWLock::RLocker locker(m_lock); - lock_required = m_require_lock_on_read; - } - + RWLock::RLocker owner_locker(m_image_ctx.owner_lock); if (m_image_ctx.non_blocking_aio || writes_blocked() || !writes_empty() || - lock_required) { + require_lock_on_read()) { queue(new AioImageRead<>(m_image_ctx, c, off, len, buf, pbl, op_flags)); } else { c->start_op(); @@ -235,13 +268,6 @@ void AioImageRequestWQ::shut_down(Context *on_shutdown) { m_image_ctx.flush(on_shutdown); } -template -bool AioImageRequestWQ::is_lock_request_needed() const { - RWLock::RLocker locker(m_lock); - return (m_queued_writes.read() > 0 || - (m_require_lock_on_read && m_queued_reads.read() > 0)); -} - template int AioImageRequestWQ::block_writes() { C_SaferCond cond_ctx; @@ -326,27 +352,23 @@ void AioImageRequestWQ::set_require_lock(AioDirection aio_direction, template void *AioImageRequestWQ::_void_dequeue() { + CephContext *cct = m_image_ctx.cct; AioImageRequest *peek_item = this->front(); - // no IO ops available or refresh in-progress (IO stalled) - if (peek_item == nullptr || m_refresh_in_progress) { + // no queued IO requests or all IO is blocked/stalled + if (peek_item == nullptr || m_io_blockers.read() > 0) { return nullptr; } + bool lock_required; bool refresh_required = m_image_ctx.state->is_refresh_required(); { RWLock::RLocker locker(m_lock); - if (peek_item->is_write_op()) { - if (m_write_blockers > 0) { - return nullptr; - } - - // refresh will requeue the op -- don't count it as in-progress - if (!refresh_required) { - m_in_flight_writes.inc(); - } - } else if (m_require_lock_on_read) { - return nullptr; + bool write_op = peek_item->is_write_op(); + lock_required = is_lock_required(write_op); + if (write_op && !lock_required && !refresh_required) { + // completed ops will requeue the IO -- don't count it as in-progress + m_in_flight_writes.inc(); } } @@ -354,12 +376,39 @@ void *AioImageRequestWQ::_void_dequeue() { ThreadPool::PointerWQ >::_void_dequeue()); assert(peek_item == item); + if (lock_required) { + this->get_pool_lock().Unlock(); + m_image_ctx.owner_lock.get_read(); + if (m_image_ctx.exclusive_lock != nullptr) { + ldout(cct, 5) << "exclusive lock required: delaying IO " << item << dendl; + if (!m_image_ctx.get_exclusive_lock_policy()->may_auto_request_lock()) { + lderr(cct) << "op requires exclusive lock" << dendl; + fail_in_flight_io(-EROFS, item); + + // wake up the IO since we won't be returning a request to process + this->signal(); + } else { + // stall IO until the acquire completes + m_io_blockers.inc(); + m_image_ctx.exclusive_lock->request_lock(new C_AcquireLock(this, item)); + } + } else { + // raced with the exclusive lock being disabled + lock_required = false; + } + m_image_ctx.owner_lock.put_read(); + this->get_pool_lock().Lock(); + + if (lock_required) { + return nullptr; + } + } + if (refresh_required) { - ldout(m_image_ctx.cct, 15) << "image refresh required: delaying IO " << item - << dendl; + ldout(cct, 5) << "image refresh required: delaying IO " << item << dendl; // stall IO until the refresh completes - m_refresh_in_progress = true; + m_io_blockers.inc(); this->get_pool_lock().Unlock(); m_image_ctx.state->refresh(new C_RefreshFinish(this, item)); @@ -455,73 +504,72 @@ void AioImageRequestWQ::finish_in_flight_io() { } template -bool AioImageRequestWQ::is_lock_required() const { - assert(m_image_ctx.owner_lock.is_locked()); - if (m_image_ctx.exclusive_lock == NULL) { - return false; - } +void AioImageRequestWQ::fail_in_flight_io(int r, AioImageRequest *req) { + this->process_finish(); + req->fail(r); + finish_queued_io(req); + delete req; + finish_in_flight_io(); +} - return (!m_image_ctx.exclusive_lock->is_lock_owner()); +template +bool AioImageRequestWQ::is_lock_required(bool write_op) const { + assert(m_lock.is_locked()); + return ((write_op && m_require_lock_on_write) || + (!write_op && m_require_lock_on_read)); } template void AioImageRequestWQ::queue(AioImageRequest *req) { + assert(m_image_ctx.owner_lock.is_locked()); + CephContext *cct = m_image_ctx.cct; ldout(cct, 20) << __func__ << ": ictx=" << &m_image_ctx << ", " << "req=" << req << dendl; - assert(m_image_ctx.owner_lock.is_locked()); - bool write_op = req->is_write_op(); - bool lock_required = (write_op && is_lock_required()) || - (!write_op && m_require_lock_on_read); - - if (lock_required && !m_image_ctx.get_exclusive_lock_policy()->may_auto_request_lock()) { - lderr(cct) << "op requires exclusive lock" << dendl; - req->fail(-EROFS); - delete req; - finish_in_flight_io(); - return; - } - - if (write_op) { + if (req->is_write_op()) { m_queued_writes.inc(); } else { m_queued_reads.inc(); } ThreadPool::PointerWQ >::queue(req); +} - if (lock_required) { - m_image_ctx.exclusive_lock->request_lock(nullptr); +template +void AioImageRequestWQ::handle_acquire_lock(int r, AioImageRequest *req) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 5) << "r=" << r << ", " << "req=" << req << dendl; + + if (r < 0) { + fail_in_flight_io(r, req); + } else { + // since IO was stalled for acquire -- original IO order is preserved + // if we requeue this op for work queue processing + this->requeue(req); } + + assert(m_io_blockers.read() > 0); + m_io_blockers.dec(); + this->signal(); } template void AioImageRequestWQ::handle_refreshed(int r, AioImageRequest *req) { CephContext *cct = m_image_ctx.cct; - ldout(cct, 15) << "resuming IO after image refresh: r=" << r << ", " - << "req=" << req << dendl; + ldout(cct, 5) << "resuming IO after image refresh: r=" << r << ", " + << "req=" << req << dendl; if (r < 0) { - this->process_finish(); - req->fail(r); - finish_queued_io(req); - delete req; - finish_in_flight_io(); + fail_in_flight_io(r, req); } else { // since IO was stalled for refresh -- original IO order is preserved // if we requeue this op for work queue processing this->requeue(req); } - m_refresh_in_progress = false; + assert(m_io_blockers.read() > 0); + m_io_blockers.dec(); this->signal(); - - // refresh might have enabled exclusive lock -- IO stalled until - // we acquire the lock - RWLock::RLocker owner_locker(m_image_ctx.owner_lock); - if (is_lock_required() && is_lock_request_needed()) { - m_image_ctx.exclusive_lock->request_lock(nullptr); - } } template diff --git a/src/librbd/AioImageRequestWQ.h b/src/librbd/AioImageRequestWQ.h index c21a23de52bf..ba535d699cf5 100644 --- a/src/librbd/AioImageRequestWQ.h +++ b/src/librbd/AioImageRequestWQ.h @@ -46,9 +46,6 @@ public: void shut_down(Context *on_shutdown); - bool is_lock_required() const; - bool is_lock_request_needed() const; - inline bool writes_blocked() const { RWLock::RLocker locker(m_lock); return (m_write_blockers > 0); @@ -67,29 +64,9 @@ protected: private: typedef std::list Contexts; - struct C_RefreshFinish : public Context { - AioImageRequestWQ *aio_work_queue; - AioImageRequest *aio_image_request; - - C_RefreshFinish(AioImageRequestWQ *aio_work_queue, - AioImageRequest *aio_image_request) - : aio_work_queue(aio_work_queue), aio_image_request(aio_image_request) { - } - virtual void finish(int r) override { - aio_work_queue->handle_refreshed(r, aio_image_request); - } - }; - - struct C_BlockedWrites : public Context { - AioImageRequestWQ *aio_work_queue; - C_BlockedWrites(AioImageRequestWQ *_aio_work_queue) - : aio_work_queue(_aio_work_queue) { - } - - virtual void finish(int r) { - aio_work_queue->handle_blocked_writes(r); - } - }; + struct C_AcquireLock; + struct C_BlockedWrites; + struct C_RefreshFinish; ImageCtxT &m_image_ctx; mutable RWLock m_lock; @@ -101,12 +78,18 @@ private: atomic_t m_queued_reads {0}; atomic_t m_queued_writes {0}; atomic_t m_in_flight_ios {0}; - - bool m_refresh_in_progress = false; + atomic_t m_io_blockers {0}; bool m_shutdown = false; Context *m_on_shutdown = nullptr; + bool is_lock_required(bool write_op) const; + + inline bool require_lock_on_read() const { + RWLock::RLocker locker(m_lock); + return m_require_lock_on_read; + + } inline bool writes_empty() const { RWLock::RLocker locker(m_lock); return (m_queued_writes.read() == 0); @@ -117,9 +100,11 @@ private: int start_in_flight_io(AioCompletion *c); void finish_in_flight_io(); + void fail_in_flight_io(int r, AioImageRequest *req); void queue(AioImageRequest *req); + void handle_acquire_lock(int r, AioImageRequest *req); void handle_refreshed(int r, AioImageRequest *req); void handle_blocked_writes(int r); }; diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc index 50cd61eb6d70..f32794052622 100644 --- a/src/librbd/ExclusiveLock.cc +++ b/src/librbd/ExclusiveLock.cc @@ -646,31 +646,21 @@ void ExclusiveLock::handle_releasing_lock(int r) { template void ExclusiveLock::handle_release_lock(int r) { - bool lock_request_needed = false; - { - Mutex::Locker locker(m_lock); - ldout(m_image_ctx.cct, 10) << this << " " << __func__ << ": r=" << r - << dendl; - - assert(m_state == STATE_PRE_RELEASING || - m_state == STATE_RELEASING); - if (r >= 0) { - m_lock.Unlock(); - m_image_ctx.image_watcher->notify_released_lock(); - lock_request_needed = m_image_ctx.aio_work_queue->is_lock_request_needed(); - m_lock.Lock(); + Mutex::Locker locker(m_lock); + ldout(m_image_ctx.cct, 10) << this << " " << __func__ << ": r=" << r + << dendl; - m_cookie = ""; - m_watch_handle = 0; - } - complete_active_action(r < 0 ? STATE_LOCKED : STATE_UNLOCKED, r); - } + assert(m_state == STATE_PRE_RELEASING || + m_state == STATE_RELEASING); + if (r >= 0) { + m_lock.Unlock(); + m_image_ctx.image_watcher->notify_released_lock(); + m_lock.Lock(); - if (r >= 0 && lock_request_needed) { - // if we have blocked IO -- re-request the lock - RWLock::RLocker owner_locker(m_image_ctx.owner_lock); - request_lock(nullptr); + m_cookie = ""; + m_watch_handle = 0; } + complete_active_action(r < 0 ? STATE_LOCKED : STATE_UNLOCKED, r); } template diff --git a/src/librbd/exclusive_lock/ReleaseRequest.cc b/src/librbd/exclusive_lock/ReleaseRequest.cc index 552ca065816d..58df357fc738 100644 --- a/src/librbd/exclusive_lock/ReleaseRequest.cc +++ b/src/librbd/exclusive_lock/ReleaseRequest.cc @@ -116,6 +116,8 @@ void ReleaseRequest::send_block_writes() { { RWLock::RLocker owner_locker(m_image_ctx.owner_lock); + // setting the lock as required will automatically cause the IO + // queue to re-request the lock if any IO is queued if (m_image_ctx.clone_copy_on_read || m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) { m_image_ctx.aio_work_queue->set_require_lock(AIO_DIRECTION_BOTH, true); diff --git a/src/test/librbd/mock/MockAioImageRequestWQ.h b/src/test/librbd/mock/MockAioImageRequestWQ.h index 0f3efac64c7f..99b283c989c6 100644 --- a/src/test/librbd/mock/MockAioImageRequestWQ.h +++ b/src/test/librbd/mock/MockAioImageRequestWQ.h @@ -16,8 +16,6 @@ struct MockAioImageRequestWQ { MOCK_METHOD0(unblock_writes, void()); MOCK_METHOD2(set_require_lock, void(AioDirection, bool)); - - MOCK_CONST_METHOD0(is_lock_request_needed, bool()); }; } // namespace librbd diff --git a/src/test/librbd/test_mock_ExclusiveLock.cc b/src/test/librbd/test_mock_ExclusiveLock.cc index df74828c9778..8d6a1bb0f4e8 100644 --- a/src/test/librbd/test_mock_ExclusiveLock.cc +++ b/src/test/librbd/test_mock_ExclusiveLock.cc @@ -152,7 +152,6 @@ public: expect_unblock_writes(mock_image_ctx); } expect_notify_released_lock(mock_image_ctx); - expect_is_lock_request_needed(mock_image_ctx, false); } } @@ -182,11 +181,6 @@ public: .Times(1); } - void expect_is_lock_request_needed(MockExclusiveLockImageCtx &mock_image_ctx, bool ret) { - EXPECT_CALL(*mock_image_ctx.aio_work_queue, is_lock_request_needed()) - .WillRepeatedly(Return(ret)); - } - void expect_flush_notifies(MockExclusiveLockImageCtx &mock_image_ctx) { EXPECT_CALL(*mock_image_ctx.image_watcher, flush(_)) .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue)); @@ -566,7 +560,6 @@ TEST_F(TestMockExclusiveLock, ConcurrentRequests) { EXPECT_CALL(release, send()) .WillOnce(Notify(&wait_for_send_ctx2)); expect_notify_released_lock(mock_image_ctx); - expect_is_lock_request_needed(mock_image_ctx, false); C_SaferCond try_request_ctx1; {