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 <idryomov@gmail.com>
(cherry picked from commit 707907ea3f6ff39968666af4ba718ceef8cd8953)

diff --git a/src/librbd/image/PreRemoveRequest.cc b/src/librbd/image/PreRemoveRequest.cc
index a75b971f7ced40df9b2aea5d3a0a0091188f784d..3ad92616af0005f3a05d0396818e500177ec1fd3 100644 (file)
--- 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<I>::send() {
 
 template <typename I>
 void PreRemoveRequest<I>::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<I>::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<I>(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 08c6eb1627caf2d568c726e4e5d3f0a432f70278..1efc30db9790a3f132b9b354bb84a033acad848c 100644 (file)
--- a/src/test/librbd/image/test_mock_PreRemoveRequest.cc
+++ b/src/test/librbd/image/test_mock_PreRemoveRequest.cc
@@ -85,6 +85,7 @@ ListWatchersRequest<MockTestImageCtx> *ListWatchersRequest<MockTestImageCtx>::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 9fff3e5929684892d76491592e75f43e6ba46c43..7fa649fd533d7c33fd5a9fb4e48ee385af9be451 100644 (file)
--- a/src/test/librbd/mock/MockImageCtx.h
+++ b/src/test/librbd/mock/MockImageCtx.h
@@ -208,6 +208,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*));