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 <mgolub@suse.com>

diff --git a/src/librbd/image/PreRemoveRequest.cc b/src/librbd/image/PreRemoveRequest.cc
index ef66a6ad774898dd22b975c30de7fc0efde17bc2..7850d965b9e81ff9c7a353f11fc387e0e4c42d96 100644 (file)
--- a/src/librbd/image/PreRemoveRequest.cc
+++ b/src/librbd/image/PreRemoveRequest.cc
@@ -68,19 +68,10 @@ void PreRemoveRequest<I>::acquire_exclusive_lock() {
     m_image_ctx->set_journal_policy(new journal::DisabledPolicy());
   }
 
-  if (m_force) {
-    auto ctx = create_context_callback<
-      PreRemoveRequest<I>,
-      &PreRemoveRequest<I>::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<I>, &PreRemoveRequest<I>::handle_exclusive_lock>(this);
-
-    m_image_ctx->exclusive_lock->try_acquire_lock(ctx);
-  }
+  auto ctx = create_context_callback<
+    PreRemoveRequest<I>, &PreRemoveRequest<I>::handle_exclusive_lock>(this);
+
+  m_image_ctx->exclusive_lock->try_acquire_lock(ctx);
 }
 
 template <typename I>
@@ -89,8 +80,14 @@ void PreRemoveRequest<I>::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<I>::handle_exclusive_lock(int r) {
 }
 
 template <typename I>
-void PreRemoveRequest<I>::handle_exclusive_lock_force(int r) {
+void PreRemoveRequest<I>::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<I>,
+    &PreRemoveRequest<I>::handle_shut_down_exclusive_lock>(this);
+
+  m_exclusive_lock = m_image_ctx->exclusive_lock;
+  m_exclusive_lock->shut_down(ctx);
+}
+
+template <typename I>
+void PreRemoveRequest<I>::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 3bb6ca6434d974f63db5587709f55f210f23c972..06b3bf2f8f686a1a1bfe38cb06f38a44af4c6b9c 100644 (file)
--- a/src/librbd/image/PreRemoveRequest.h
+++ b/src/librbd/image/PreRemoveRequest.h
@@ -35,15 +35,12 @@ private:
    * @verbatim
    *
    *       <start>
-   *          |
-   *          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 131b1d801273ac9f58f01c394b8e94c69344290f..65a20db9dbee0962373c1d3f11d96eec6cb01786 100644 (file)
--- 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;