From 48ab30d9b551edbb5270972ea8cdb8caf02769a7 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Tue, 20 Aug 2019 15:39:38 +0100 Subject: [PATCH] librbd: always try to acquire exclusive lock when removing image We want it to use object map for fast removal. Fixes: https://tracker.ceph.com/issues/41229 Signed-off-by: Mykola Golub --- src/librbd/image/PreRemoveRequest.cc | 48 +++++++---- src/librbd/image/PreRemoveRequest.h | 19 ++--- .../image/test_mock_PreRemoveRequest.cc | 82 +++++++++++++++---- 3 files changed, 105 insertions(+), 44 deletions(-) diff --git a/src/librbd/image/PreRemoveRequest.cc b/src/librbd/image/PreRemoveRequest.cc index ef66a6ad7748..7850d965b9e8 100644 --- a/src/librbd/image/PreRemoveRequest.cc +++ b/src/librbd/image/PreRemoveRequest.cc @@ -68,19 +68,10 @@ void PreRemoveRequest::acquire_exclusive_lock() { m_image_ctx->set_journal_policy(new journal::DisabledPolicy()); } - if (m_force) { - auto ctx = create_context_callback< - PreRemoveRequest, - &PreRemoveRequest::handle_exclusive_lock_force>(this); - - m_exclusive_lock = m_image_ctx->exclusive_lock; - m_exclusive_lock->shut_down(ctx); - } else { - auto ctx = create_context_callback< - PreRemoveRequest, &PreRemoveRequest::handle_exclusive_lock>(this); - - m_image_ctx->exclusive_lock->try_acquire_lock(ctx); - } + auto ctx = create_context_callback< + PreRemoveRequest, &PreRemoveRequest::handle_exclusive_lock>(this); + + m_image_ctx->exclusive_lock->try_acquire_lock(ctx); } template @@ -89,8 +80,14 @@ void PreRemoveRequest::handle_exclusive_lock(int r) { ldout(cct, 5) << "r=" << r << dendl; if (r < 0 || !m_image_ctx->exclusive_lock->is_lock_owner()) { - lderr(cct) << "cannot obtain exclusive lock - not removing" << dendl; - finish(-EBUSY); + if (!m_force) { + lderr(cct) << "cannot obtain exclusive lock - not removing" << dendl; + finish(-EBUSY); + } else { + ldout(cct, 5) << "cannot obtain exclusive lock - " + << "proceeding due to force flag set" << dendl; + shut_down_exclusive_lock(); + } return; } @@ -98,7 +95,26 @@ void PreRemoveRequest::handle_exclusive_lock(int r) { } template -void PreRemoveRequest::handle_exclusive_lock_force(int r) { +void PreRemoveRequest::shut_down_exclusive_lock() { + std::shared_lock owner_lock{m_image_ctx->owner_lock}; + if (m_image_ctx->exclusive_lock == nullptr) { + validate_image_removal(); + return; + } + + auto cct = m_image_ctx->cct; + ldout(cct, 5) << dendl; + + auto ctx = create_context_callback< + PreRemoveRequest, + &PreRemoveRequest::handle_shut_down_exclusive_lock>(this); + + m_exclusive_lock = m_image_ctx->exclusive_lock; + m_exclusive_lock->shut_down(ctx); +} + +template +void PreRemoveRequest::handle_shut_down_exclusive_lock(int r) { auto cct = m_image_ctx->cct; ldout(cct, 5) << "r=" << r << dendl; diff --git a/src/librbd/image/PreRemoveRequest.h b/src/librbd/image/PreRemoveRequest.h index 3bb6ca6434d9..06b3bf2f8f68 100644 --- a/src/librbd/image/PreRemoveRequest.h +++ b/src/librbd/image/PreRemoveRequest.h @@ -35,15 +35,12 @@ private: * @verbatim * * - * | - * v - * CHECK EXCLUSIVE LOCK - * | - * v (skip if not needed) - * ACQUIRE EXCLUSIVE LOCK - * | - * v - * CHECK IMAGE WATCHERS + * | (skip if + * v not needed) (error) + * ACQUIRE EXCLUSIVE LOCK * * * * * * > SHUT DOWN EXCLUSIVE LOCK + * | | + * v | + * CHECK IMAGE WATCHERS <------------------/ * | * v * CHECK GROUP @@ -73,7 +70,9 @@ private: void acquire_exclusive_lock(); void handle_exclusive_lock(int r); - void handle_exclusive_lock_force(int r); + + void shut_down_exclusive_lock(); + void handle_shut_down_exclusive_lock(int r); void validate_image_removal(); void check_image_snaps(); diff --git a/src/test/librbd/image/test_mock_PreRemoveRequest.cc b/src/test/librbd/image/test_mock_PreRemoveRequest.cc index 131b1d801273..65a20db9dbee 100644 --- a/src/test/librbd/image/test_mock_PreRemoveRequest.cc +++ b/src/test/librbd/image/test_mock_PreRemoveRequest.cc @@ -203,9 +203,9 @@ public: }; TEST_F(TestMockImagePreRemoveRequest, Success) { - MockExclusiveLock *mock_exclusive_lock = new MockExclusiveLock(); + MockExclusiveLock mock_exclusive_lock; if (m_test_imctx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) { - m_mock_imctx->exclusive_lock = mock_exclusive_lock; + m_mock_imctx->exclusive_lock = &mock_exclusive_lock; } expect_op_work_queue(*m_mock_imctx); @@ -213,7 +213,8 @@ TEST_F(TestMockImagePreRemoveRequest, Success) { InSequence seq; expect_set_journal_policy(*m_mock_imctx); - expect_shut_down_exclusive_lock(*m_mock_imctx, *mock_exclusive_lock, 0); + expect_try_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, 0); + expect_is_exclusive_lock_owner(*m_mock_imctx, mock_exclusive_lock, true); MockListWatchersRequest mock_list_watchers_request; expect_list_image_watchers(*m_mock_imctx, mock_list_watchers_request, 0); @@ -221,7 +222,7 @@ TEST_F(TestMockImagePreRemoveRequest, Success) { expect_get_group(*m_mock_imctx, 0); C_SaferCond ctx; - auto req = MockPreRemoveRequest::create(m_mock_imctx, true, &ctx); + auto req = MockPreRemoveRequest::create(m_mock_imctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -239,43 +240,40 @@ TEST_F(TestMockImagePreRemoveRequest, OperationsDisabled) { ASSERT_EQ(-EROFS, ctx.wait()); } -TEST_F(TestMockImagePreRemoveRequest, ExclusiveLockShutDownFailed) { +TEST_F(TestMockImagePreRemoveRequest, ExclusiveLockTryAcquireFailed) { REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); - MockExclusiveLock *mock_exclusive_lock = new MockExclusiveLock(); - if (m_test_imctx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) { - m_mock_imctx->exclusive_lock = mock_exclusive_lock; - } + MockExclusiveLock mock_exclusive_lock; + m_mock_imctx->exclusive_lock = &mock_exclusive_lock; expect_op_work_queue(*m_mock_imctx); expect_test_features(*m_mock_imctx); InSequence seq; expect_set_journal_policy(*m_mock_imctx); - expect_shut_down_exclusive_lock(*m_mock_imctx, *mock_exclusive_lock, -EINVAL); + expect_try_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, + -EINVAL); C_SaferCond ctx; - auto req = MockPreRemoveRequest::create(m_mock_imctx, true, &ctx); + auto req = MockPreRemoveRequest::create(m_mock_imctx, false, &ctx); req->send(); - ASSERT_EQ(-EINVAL, ctx.wait()); + ASSERT_EQ(-EBUSY, ctx.wait()); } -TEST_F(TestMockImagePreRemoveRequest, ExclusiveLockTryAcquireFailed) { +TEST_F(TestMockImagePreRemoveRequest, ExclusiveLockTryAcquireNotLockOwner) { REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); MockExclusiveLock mock_exclusive_lock; - if (m_test_imctx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) { - m_mock_imctx->exclusive_lock = &mock_exclusive_lock; - } + m_mock_imctx->exclusive_lock = &mock_exclusive_lock; expect_op_work_queue(*m_mock_imctx); expect_test_features(*m_mock_imctx); InSequence seq; expect_set_journal_policy(*m_mock_imctx); - expect_try_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, - -EINVAL); + expect_try_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, 0); + expect_is_exclusive_lock_owner(*m_mock_imctx, mock_exclusive_lock, false); C_SaferCond ctx; auto req = MockPreRemoveRequest::create(m_mock_imctx, false, &ctx); @@ -284,6 +282,54 @@ TEST_F(TestMockImagePreRemoveRequest, ExclusiveLockTryAcquireFailed) { ASSERT_EQ(-EBUSY, ctx.wait()); } +TEST_F(TestMockImagePreRemoveRequest, Force) { + REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); + + MockExclusiveLock *mock_exclusive_lock = new MockExclusiveLock(); + m_mock_imctx->exclusive_lock = mock_exclusive_lock; + + expect_op_work_queue(*m_mock_imctx); + expect_test_features(*m_mock_imctx); + + InSequence seq; + expect_set_journal_policy(*m_mock_imctx); + expect_try_acquire_exclusive_lock(*m_mock_imctx, *mock_exclusive_lock, + -EINVAL); + expect_shut_down_exclusive_lock(*m_mock_imctx, *mock_exclusive_lock, 0); + + MockListWatchersRequest mock_list_watchers_request; + expect_list_image_watchers(*m_mock_imctx, mock_list_watchers_request, 0); + + expect_get_group(*m_mock_imctx, 0); + + C_SaferCond ctx; + auto req = MockPreRemoveRequest::create(m_mock_imctx, true, &ctx); + req->send(); + + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockImagePreRemoveRequest, ExclusiveLockShutDownFailed) { + REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); + + MockExclusiveLock *mock_exclusive_lock = new MockExclusiveLock(); + m_mock_imctx->exclusive_lock = mock_exclusive_lock; + + expect_op_work_queue(*m_mock_imctx); + expect_test_features(*m_mock_imctx); + + InSequence seq; + expect_set_journal_policy(*m_mock_imctx); + expect_try_acquire_exclusive_lock(*m_mock_imctx, *mock_exclusive_lock, -EINVAL); + expect_shut_down_exclusive_lock(*m_mock_imctx, *mock_exclusive_lock, -EINVAL); + + C_SaferCond ctx; + auto req = MockPreRemoveRequest::create(m_mock_imctx, true, &ctx); + req->send(); + + ASSERT_EQ(-EINVAL, ctx.wait()); +} + TEST_F(TestMockImagePreRemoveRequest, Migration) { m_mock_imctx->features |= RBD_FEATURE_MIGRATING; -- 2.47.3