From 8385912b5e67f12cb45557abffd542ec1059fe95 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Sun, 7 Feb 2021 13:46:15 +0100 Subject: [PATCH] librbd: refuse to release exclusive lock when removing Commit 25c2ffe145be ("librbd: acquire exclusive lock from peer when removing") changed PreRemoveRequest to request exclusive lock from the peer instead of giving up and proceeding without exclusive lock. This caused one of the test cases that sometimes runs concurrent "rbd rm" against the same image to fail intermittently, most often on assert ceph_assert(image_ctx.exclusive_lock == nullptr || image_ctx.exclusive_lock->is_lock_owner()); because exclusive lock is now automatically transitioned to another "rbd rm" on its request. The root cause is older and probably goes back to when synchronous librbd::remove() which held owner_lock across all operations including trim_image() was converted to a set of state machines. Since then, any peer that requests exclusive lock (instead of trying once and backing off) is able to mess with image removal. Install StandardPolicy to disable automatic exclusive lock transitions during image removal. Fixes: https://tracker.ceph.com/issues/49226 Signed-off-by: Ilya Dryomov (cherry picked from commit 707907ea3f6ff39968666af4ba718ceef8cd8953) --- src/librbd/image/PreRemoveRequest.cc | 10 +++++++++- .../image/test_mock_PreRemoveRequest.cc | 19 +++++++++++++++++++ src/test/librbd/mock/MockImageCtx.h | 1 + 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/librbd/image/PreRemoveRequest.cc b/src/librbd/image/PreRemoveRequest.cc index a75b971f7ced4..3ad92616af000 100644 --- a/src/librbd/image/PreRemoveRequest.cc +++ b/src/librbd/image/PreRemoveRequest.cc @@ -7,6 +7,7 @@ #include "cls/rbd/cls_rbd_types.h" #include "librbd/ExclusiveLock.h" #include "librbd/Utils.h" +#include "librbd/exclusive_lock/StandardPolicy.h" #include "librbd/image/ListWatchersRequest.h" #include "librbd/journal/DisabledPolicy.h" #include "librbd/operation/SnapshotRemoveRequest.h" @@ -63,8 +64,10 @@ void PreRemoveRequest::send() { template void PreRemoveRequest::acquire_exclusive_lock() { - std::shared_lock owner_lock{m_image_ctx->owner_lock}; + // lock for write for set_exclusive_lock_policy() + std::unique_lock owner_locker{m_image_ctx->owner_lock}; if (m_image_ctx->exclusive_lock == nullptr) { + owner_locker.unlock(); validate_image_removal(); return; } @@ -72,6 +75,11 @@ void PreRemoveRequest::acquire_exclusive_lock() { auto cct = m_image_ctx->cct; ldout(cct, 5) << dendl; + // refuse to release exclusive lock when (in the midst of) removing + // the image + m_image_ctx->set_exclusive_lock_policy( + new exclusive_lock::StandardPolicy(m_image_ctx)); + // do not attempt to open the journal when removing the image in case // it's corrupt if (m_image_ctx->test_features(RBD_FEATURE_JOURNALING)) { diff --git a/src/test/librbd/image/test_mock_PreRemoveRequest.cc b/src/test/librbd/image/test_mock_PreRemoveRequest.cc index a451104f882fc..faae4f2017f89 100644 --- a/src/test/librbd/image/test_mock_PreRemoveRequest.cc +++ b/src/test/librbd/image/test_mock_PreRemoveRequest.cc @@ -85,6 +85,7 @@ ListWatchersRequest *ListWatchersRequest::s_ } // namespace librbd // template definitions +#include "librbd/exclusive_lock/StandardPolicy.cc" #include "librbd/image/PreRemoveRequest.cc" ACTION_P(TestFeatures, image_ctx) { @@ -132,6 +133,16 @@ public: .WillRepeatedly(TestFeatures(&mock_image_ctx)); } + void expect_set_exclusive_lock_policy(MockTestImageCtx& mock_image_ctx) { + if (m_mock_imctx->exclusive_lock != nullptr) { + EXPECT_CALL(mock_image_ctx, set_exclusive_lock_policy(_)) + .WillOnce(Invoke([](exclusive_lock::Policy* policy) { + ASSERT_FALSE(policy->may_auto_request_lock()); + delete policy; + })); + } + } + void expect_set_journal_policy(MockTestImageCtx &mock_image_ctx) { if (m_test_imctx->test_features(RBD_FEATURE_JOURNALING)) { EXPECT_CALL(mock_image_ctx, set_journal_policy(_)) @@ -212,6 +223,7 @@ TEST_F(TestMockImagePreRemoveRequest, Success) { expect_test_features(*m_mock_imctx); InSequence seq; + expect_set_exclusive_lock_policy(*m_mock_imctx); expect_set_journal_policy(*m_mock_imctx); expect_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, 0); expect_is_exclusive_lock_owner(*m_mock_imctx, mock_exclusive_lock, true); @@ -250,6 +262,7 @@ TEST_F(TestMockImagePreRemoveRequest, ExclusiveLockTryAcquireFailed) { expect_test_features(*m_mock_imctx); InSequence seq; + expect_set_exclusive_lock_policy(*m_mock_imctx); expect_set_journal_policy(*m_mock_imctx); expect_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, -EINVAL); @@ -271,6 +284,7 @@ TEST_F(TestMockImagePreRemoveRequest, ExclusiveLockTryAcquireNotLockOwner) { expect_test_features(*m_mock_imctx); InSequence seq; + expect_set_exclusive_lock_policy(*m_mock_imctx); expect_set_journal_policy(*m_mock_imctx); expect_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, 0); expect_is_exclusive_lock_owner(*m_mock_imctx, mock_exclusive_lock, false); @@ -292,6 +306,7 @@ TEST_F(TestMockImagePreRemoveRequest, Force) { expect_test_features(*m_mock_imctx); InSequence seq; + expect_set_exclusive_lock_policy(*m_mock_imctx); expect_set_journal_policy(*m_mock_imctx); expect_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, -EINVAL); @@ -319,6 +334,7 @@ TEST_F(TestMockImagePreRemoveRequest, ExclusiveLockShutDownFailed) { expect_test_features(*m_mock_imctx); InSequence seq; + expect_set_exclusive_lock_policy(*m_mock_imctx); expect_set_journal_policy(*m_mock_imctx); expect_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, -EINVAL); expect_shut_down_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, -EINVAL); @@ -365,6 +381,7 @@ TEST_F(TestMockImagePreRemoveRequest, Watchers) { expect_test_features(*m_mock_imctx); InSequence seq; + expect_set_exclusive_lock_policy(*m_mock_imctx); expect_set_journal_policy(*m_mock_imctx); expect_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, 0); expect_is_exclusive_lock_owner(*m_mock_imctx, mock_exclusive_lock, true); @@ -392,6 +409,7 @@ TEST_F(TestMockImagePreRemoveRequest, GroupError) { expect_test_features(*m_mock_imctx); InSequence seq; + expect_set_exclusive_lock_policy(*m_mock_imctx); expect_set_journal_policy(*m_mock_imctx); expect_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, 0); expect_is_exclusive_lock_owner(*m_mock_imctx, mock_exclusive_lock, true); @@ -423,6 +441,7 @@ TEST_F(TestMockImagePreRemoveRequest, AutoDeleteSnapshots) { {123, {"snap1", {cls::rbd::TrashSnapshotNamespace{}}, {}, {}, {}, {}, {}}}}; InSequence seq; + expect_set_exclusive_lock_policy(*m_mock_imctx); expect_set_journal_policy(*m_mock_imctx); expect_acquire_exclusive_lock(*m_mock_imctx, mock_exclusive_lock, 0); expect_is_exclusive_lock_owner(*m_mock_imctx, mock_exclusive_lock, true); diff --git a/src/test/librbd/mock/MockImageCtx.h b/src/test/librbd/mock/MockImageCtx.h index dcbd7c21a1265..80307962eb4fd 100644 --- a/src/test/librbd/mock/MockImageCtx.h +++ b/src/test/librbd/mock/MockImageCtx.h @@ -214,6 +214,7 @@ struct MockImageCtx { MOCK_METHOD1(notify_update, void(Context *)); MOCK_CONST_METHOD0(get_exclusive_lock_policy, exclusive_lock::Policy*()); + MOCK_METHOD1(set_exclusive_lock_policy, void(exclusive_lock::Policy*)); MOCK_CONST_METHOD0(get_journal_policy, journal::Policy*()); MOCK_METHOD1(set_journal_policy, void(journal::Policy*)); -- 2.39.5