From ebce8ceb9353052d1d43d18e2bb76c68e581272e Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 22 Jun 2016 10:13:45 -0400 Subject: [PATCH 1/2] librbd: optionally block proxied requests with an error code Signed-off-by: Jason Dillaman (cherry picked from commit 93e2faf38e866fb3e32a7b3f3527d97215c60d31) --- src/librbd/ExclusiveLock.cc | 23 +- src/librbd/ExclusiveLock.h | 7 +- src/librbd/ImageWatcher.cc | 256 ++++++++++++--------- src/librbd/Operations.cc | 5 +- src/librbd/internal.cc | 2 +- src/test/librbd/test_mock_ExclusiveLock.cc | 38 +++ 6 files changed, 208 insertions(+), 123 deletions(-) diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc index 932fe042d3e79..c3ba7c72830a1 100644 --- a/src/librbd/ExclusiveLock.cc +++ b/src/librbd/ExclusiveLock.cc @@ -75,33 +75,36 @@ bool ExclusiveLock::is_lock_owner() const { } template -bool ExclusiveLock::accept_requests() const { +bool ExclusiveLock::accept_requests(int *ret_val) const { Mutex::Locker locker(m_lock); bool accept_requests = (!is_shutdown() && m_state == STATE_LOCKED && - m_request_blockers == 0); + !m_request_blocked); + *ret_val = m_request_blocked_ret_val; + ldout(m_image_ctx.cct, 20) << this << " " << __func__ << "=" << accept_requests << dendl; return accept_requests; } template -void ExclusiveLock::block_requests() { +void ExclusiveLock::block_requests(int r) { Mutex::Locker locker(m_lock); - ++m_request_blockers; + assert(!m_request_blocked); + m_request_blocked = true; + m_request_blocked_ret_val = r; - ldout(m_image_ctx.cct, 20) << this << " " << __func__ << "=" - << m_request_blockers << dendl; + ldout(m_image_ctx.cct, 20) << this << " " << __func__ << dendl; } template void ExclusiveLock::unblock_requests() { Mutex::Locker locker(m_lock); - assert(m_request_blockers > 0); - --m_request_blockers; + assert(m_request_blocked); + m_request_blocked = false; + m_request_blocked_ret_val = 0; - ldout(m_image_ctx.cct, 20) << this << " " << __func__ << "=" - << m_request_blockers << dendl; + ldout(m_image_ctx.cct, 20) << this << " " << __func__ << dendl; } template diff --git a/src/librbd/ExclusiveLock.h b/src/librbd/ExclusiveLock.h index 6268c8080be11..0a2a88e294968 100644 --- a/src/librbd/ExclusiveLock.h +++ b/src/librbd/ExclusiveLock.h @@ -30,9 +30,9 @@ class ExclusiveLock { ~ExclusiveLock(); bool is_lock_owner() const; - bool accept_requests() const; + bool accept_requests(int *ret_val) const; - void block_requests(); + void block_requests(int r); void unblock_requests(); void init(uint64_t features, Context *on_init); @@ -130,7 +130,8 @@ class ExclusiveLock { ActionsContexts m_actions_contexts; - uint32_t m_request_blockers = 0; + bool m_request_blocked = false; + int m_request_blocked_ret_val = 0; std::string encode_lock_cookie() const; diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc index d3d4e701a736e..f7531bfcaa6b3 100644 --- a/src/librbd/ImageWatcher.cc +++ b/src/librbd/ImageWatcher.cc @@ -613,21 +613,25 @@ bool ImageWatcher::handle_payload(const RequestLockPayload &payload, } RWLock::RLocker l(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - // need to send something back so the client can detect a missing leader - ::encode(ResponseMessage(0), ack_ctx->out); - - { - Mutex::Locker owner_client_id_locker(m_owner_client_id_lock); - if (!m_owner_client_id.is_valid()) { - return true; + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + // need to send something back so the client can detect a missing leader + ::encode(ResponseMessage(0), ack_ctx->out); + + { + Mutex::Locker owner_client_id_locker(m_owner_client_id_lock); + if (!m_owner_client_id.is_valid()) { + return true; + } } - } - ldout(m_image_ctx.cct, 10) << this << " queuing release of exclusive lock" - << dendl; - m_image_ctx.get_exclusive_lock_policy()->lock_requested(payload.force); + ldout(m_image_ctx.cct, 10) << this << " queuing release of exclusive lock" + << dendl; + m_image_ctx.get_exclusive_lock_policy()->lock_requested(payload.force); + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -664,20 +668,24 @@ bool ImageWatcher::handle_payload(const FlattenPayload &payload, C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - bool new_request; - Context *ctx; - ProgressContext *prog_ctx; - int r = prepare_async_request(payload.async_request_id, &new_request, - &ctx, &prog_ctx); - if (new_request) { - ldout(m_image_ctx.cct, 10) << this << " remote flatten request: " - << payload.async_request_id << dendl; - m_image_ctx.operations->execute_flatten(*prog_ctx, ctx); - } + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + bool new_request; + Context *ctx; + ProgressContext *prog_ctx; + r = prepare_async_request(payload.async_request_id, &new_request, + &ctx, &prog_ctx); + if (new_request) { + ldout(m_image_ctx.cct, 10) << this << " remote flatten request: " + << payload.async_request_id << dendl; + m_image_ctx.operations->execute_flatten(*prog_ctx, ctx); + } - ::encode(ResponseMessage(r), ack_ctx->out); + ::encode(ResponseMessage(r), ack_ctx->out); + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -685,21 +693,25 @@ bool ImageWatcher::handle_payload(const FlattenPayload &payload, bool ImageWatcher::handle_payload(const ResizePayload &payload, C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - bool new_request; - Context *ctx; - ProgressContext *prog_ctx; - int r = prepare_async_request(payload.async_request_id, &new_request, - &ctx, &prog_ctx); - if (new_request) { - ldout(m_image_ctx.cct, 10) << this << " remote resize request: " - << payload.async_request_id << " " - << payload.size << dendl; - m_image_ctx.operations->execute_resize(payload.size, *prog_ctx, ctx, 0); - } + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + bool new_request; + Context *ctx; + ProgressContext *prog_ctx; + r = prepare_async_request(payload.async_request_id, &new_request, + &ctx, &prog_ctx); + if (new_request) { + ldout(m_image_ctx.cct, 10) << this << " remote resize request: " + << payload.async_request_id << " " + << payload.size << dendl; + m_image_ctx.operations->execute_resize(payload.size, *prog_ctx, ctx, 0); + } - ::encode(ResponseMessage(r), ack_ctx->out); + ::encode(ResponseMessage(r), ack_ctx->out); + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -707,15 +719,19 @@ bool ImageWatcher::handle_payload(const ResizePayload &payload, bool ImageWatcher::handle_payload(const SnapCreatePayload &payload, C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - ldout(m_image_ctx.cct, 10) << this << " remote snap_create request: " - << payload.snap_name << dendl; - - m_image_ctx.operations->execute_snap_create(payload.snap_name.c_str(), - new C_ResponseMessage(ack_ctx), - 0, false); - return false; + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + ldout(m_image_ctx.cct, 10) << this << " remote snap_create request: " + << payload.snap_name << dendl; + + m_image_ctx.operations->execute_snap_create(payload.snap_name.c_str(), + new C_ResponseMessage(ack_ctx), + 0, false); + return false; + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -723,16 +739,20 @@ bool ImageWatcher::handle_payload(const SnapCreatePayload &payload, bool ImageWatcher::handle_payload(const SnapRenamePayload &payload, C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - ldout(m_image_ctx.cct, 10) << this << " remote snap_rename request: " - << payload.snap_id << " to " - << payload.snap_name << dendl; - - m_image_ctx.operations->execute_snap_rename(payload.snap_id, - payload.snap_name.c_str(), - new C_ResponseMessage(ack_ctx)); - return false; + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + ldout(m_image_ctx.cct, 10) << this << " remote snap_rename request: " + << payload.snap_id << " to " + << payload.snap_name << dendl; + + m_image_ctx.operations->execute_snap_rename(payload.snap_id, + payload.snap_name.c_str(), + new C_ResponseMessage(ack_ctx)); + return false; + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -740,14 +760,18 @@ bool ImageWatcher::handle_payload(const SnapRenamePayload &payload, bool ImageWatcher::handle_payload(const SnapRemovePayload &payload, C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - ldout(m_image_ctx.cct, 10) << this << " remote snap_remove request: " - << payload.snap_name << dendl; - - m_image_ctx.operations->execute_snap_remove(payload.snap_name.c_str(), - new C_ResponseMessage(ack_ctx)); - return false; + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + ldout(m_image_ctx.cct, 10) << this << " remote snap_remove request: " + << payload.snap_name << dendl; + + m_image_ctx.operations->execute_snap_remove(payload.snap_name.c_str(), + new C_ResponseMessage(ack_ctx)); + return false; + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -755,14 +779,18 @@ bool ImageWatcher::handle_payload(const SnapRemovePayload &payload, bool ImageWatcher::handle_payload(const SnapProtectPayload& payload, C_NotifyAck *ack_ctx) { RWLock::RLocker owner_locker(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - ldout(m_image_ctx.cct, 10) << this << " remote snap_protect request: " - << payload.snap_name << dendl; + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + ldout(m_image_ctx.cct, 10) << this << " remote snap_protect request: " + << payload.snap_name << dendl; - m_image_ctx.operations->execute_snap_protect(payload.snap_name.c_str(), - new C_ResponseMessage(ack_ctx)); - return false; + m_image_ctx.operations->execute_snap_protect(payload.snap_name.c_str(), + new C_ResponseMessage(ack_ctx)); + return false; + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -770,14 +798,18 @@ bool ImageWatcher::handle_payload(const SnapProtectPayload& payload, bool ImageWatcher::handle_payload(const SnapUnprotectPayload& payload, C_NotifyAck *ack_ctx) { RWLock::RLocker owner_locker(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - ldout(m_image_ctx.cct, 10) << this << " remote snap_unprotect request: " - << payload.snap_name << dendl; - - m_image_ctx.operations->execute_snap_unprotect(payload.snap_name.c_str(), - new C_ResponseMessage(ack_ctx)); - return false; + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + ldout(m_image_ctx.cct, 10) << this << " remote snap_unprotect request: " + << payload.snap_name << dendl; + + m_image_ctx.operations->execute_snap_unprotect(payload.snap_name.c_str(), + new C_ResponseMessage(ack_ctx)); + return false; + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -785,21 +817,25 @@ bool ImageWatcher::handle_payload(const SnapUnprotectPayload& payload, bool ImageWatcher::handle_payload(const RebuildObjectMapPayload& payload, C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - bool new_request; - Context *ctx; - ProgressContext *prog_ctx; - int r = prepare_async_request(payload.async_request_id, &new_request, - &ctx, &prog_ctx); - if (new_request) { - ldout(m_image_ctx.cct, 10) << this - << " remote rebuild object map request: " - << payload.async_request_id << dendl; - m_image_ctx.operations->execute_rebuild_object_map(*prog_ctx, ctx); - } + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + bool new_request; + Context *ctx; + ProgressContext *prog_ctx; + r = prepare_async_request(payload.async_request_id, &new_request, + &ctx, &prog_ctx); + if (new_request) { + ldout(m_image_ctx.cct, 10) << this + << " remote rebuild object map request: " + << payload.async_request_id << dendl; + m_image_ctx.operations->execute_rebuild_object_map(*prog_ctx, ctx); + } - ::encode(ResponseMessage(r), ack_ctx->out); + ::encode(ResponseMessage(r), ack_ctx->out); + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -807,14 +843,18 @@ bool ImageWatcher::handle_payload(const RebuildObjectMapPayload& payload, bool ImageWatcher::handle_payload(const RenamePayload& payload, C_NotifyAck *ack_ctx) { RWLock::RLocker owner_locker(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - ldout(m_image_ctx.cct, 10) << this << " remote rename request: " - << payload.image_name << dendl; - - m_image_ctx.operations->execute_rename(payload.image_name.c_str(), - new C_ResponseMessage(ack_ctx)); - return false; + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r)) { + ldout(m_image_ctx.cct, 10) << this << " remote rename request: " + << payload.image_name << dendl; + + m_image_ctx.operations->execute_rename(payload.image_name.c_str(), + new C_ResponseMessage(ack_ctx)); + return false; + } else if (r < 0) { + ::encode(ResponseMessage(r), ack_ctx->out); + } } return true; } @@ -822,9 +862,11 @@ bool ImageWatcher::handle_payload(const RenamePayload& payload, bool ImageWatcher::handle_payload(const UnknownPayload &payload, C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (m_image_ctx.exclusive_lock != nullptr && - m_image_ctx.exclusive_lock->accept_requests()) { - ::encode(ResponseMessage(-EOPNOTSUPP), ack_ctx->out); + if (m_image_ctx.exclusive_lock != nullptr) { + int r; + if (m_image_ctx.exclusive_lock->accept_requests(&r) || r < 0) { + ::encode(ResponseMessage(-EOPNOTSUPP), ack_ctx->out); + } } return true; } diff --git a/src/librbd/Operations.cc b/src/librbd/Operations.cc index 0fa190b1dcaac..28afa4dda0f99 100644 --- a/src/librbd/Operations.cc +++ b/src/librbd/Operations.cc @@ -176,8 +176,9 @@ struct C_InvokeAsyncRequest : public Context { return; } + int r; if (image_ctx.exclusive_lock->is_lock_owner() && - image_ctx.exclusive_lock->accept_requests()) { + image_ctx.exclusive_lock->accept_requests(&r)) { send_local_request(); owner_lock.put_read(); return; @@ -1050,7 +1051,7 @@ int Operations::prepare_image_update() { RWLock::WLocker owner_locker(m_image_ctx.owner_lock); if (m_image_ctx.exclusive_lock != nullptr && (!m_image_ctx.exclusive_lock->is_lock_owner() || - !m_image_ctx.exclusive_lock->accept_requests())) { + !m_image_ctx.exclusive_lock->accept_requests(&r))) { m_image_ctx.exclusive_lock->try_lock(&ctx); trying_lock = true; } diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index 13682df4182ab..2518369d7c427 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -1689,7 +1689,7 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, // avoid accepting new requests from peers while we manipulate // the image features if (ictx->exclusive_lock != nullptr) { - ictx->exclusive_lock->block_requests(); + ictx->exclusive_lock->block_requests(0); } BOOST_SCOPE_EXIT_ALL( (ictx) ) { if (ictx->exclusive_lock != nullptr) { diff --git a/src/test/librbd/test_mock_ExclusiveLock.cc b/src/test/librbd/test_mock_ExclusiveLock.cc index 775767598c706..e508c8d202710 100644 --- a/src/test/librbd/test_mock_ExclusiveLock.cc +++ b/src/test/librbd/test_mock_ExclusiveLock.cc @@ -589,5 +589,43 @@ TEST_F(TestMockExclusiveLock, ConcurrentRequests) { ASSERT_EQ(0, when_shut_down(mock_image_ctx, exclusive_lock)); } +TEST_F(TestMockExclusiveLock, BlockRequests) { + REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockExclusiveLockImageCtx mock_image_ctx(*ictx); + MockExclusiveLock exclusive_lock(mock_image_ctx); + + expect_op_work_queue(mock_image_ctx); + + InSequence seq; + expect_block_writes(mock_image_ctx); + ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock)); + + MockAcquireRequest try_lock_acquire; + expect_acquire_lock(mock_image_ctx, try_lock_acquire, 0); + ASSERT_EQ(0, when_try_lock(mock_image_ctx, exclusive_lock)); + ASSERT_TRUE(is_lock_owner(mock_image_ctx, exclusive_lock)); + + int ret_val; + ASSERT_TRUE(exclusive_lock.accept_requests(&ret_val)); + ASSERT_EQ(0, ret_val); + + exclusive_lock.block_requests(-EROFS); + ASSERT_FALSE(exclusive_lock.accept_requests(&ret_val)); + ASSERT_EQ(-EROFS, ret_val); + + exclusive_lock.unblock_requests(); + ASSERT_TRUE(exclusive_lock.accept_requests(&ret_val)); + ASSERT_EQ(0, ret_val); + + MockReleaseRequest shutdown_release; + expect_release_lock(mock_image_ctx, shutdown_release, 0, true); + ASSERT_EQ(0, when_shut_down(mock_image_ctx, exclusive_lock)); + ASSERT_FALSE(is_lock_owner(mock_image_ctx, exclusive_lock)); +} + } // namespace librbd From db28ddcf88c13aef80e5a7131db463b305102abe Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 22 Jun 2016 10:14:21 -0400 Subject: [PATCH 2/2] rbd-mirror: block proxied ops with -EROFS return code When replicating to a local image, the daemon will own the exclusive lock and will receive any proxied maintenance ops from other clients. Since the image is non-primary, respond with -EROFS. Fixes: http://tracker.ceph.com/issues/16411 Signed-off-by: Jason Dillaman (cherry picked from commit 07b49df24e5f30460ce3ab584a89370ea3ff7cc8) --- src/tools/rbd_mirror/image_replayer/OpenLocalImageRequest.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tools/rbd_mirror/image_replayer/OpenLocalImageRequest.cc b/src/tools/rbd_mirror/image_replayer/OpenLocalImageRequest.cc index 9367ed6dfa96c..35b6863098b46 100644 --- a/src/tools/rbd_mirror/image_replayer/OpenLocalImageRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/OpenLocalImageRequest.cc @@ -141,6 +141,9 @@ void OpenLocalImageRequest::send_lock_image() { return; } + // disallow any proxied maintenance operations before grabbing lock + (*m_local_image_ctx)->exclusive_lock->block_requests(-EROFS); + Context *ctx = create_context_callback< OpenLocalImageRequest, &OpenLocalImageRequest::handle_lock_image>( this);