librbd: fix ExclusiveLock::accept_request() when !is_state_locked()

To accept an async request, two conditions must be met: a) exclusive
lock must be a firm STATE_LOCKED state and b) async requests shouldn't
be blocked or if they are blocked there should be an exception in place
for a given request_type.  If a) is met but b) isn't, ret_val is set
to m_request_blocked_ret_val, as expected -- the reason for denying
the request is that async requests are blocked.  However, if a) isn't
met, ret_val also gets set to m_request_blocked_ret_val.  This is wrong
because the reason for denying the request in this case isn't that
async requests are blocked (they may or may not be) but a much heavier
circumstance of exclusive lock being in a transient state or not held
at all.

In such scenarios, whether async requests are blocked or not isn't
relevant and ExclusiveLock::accept_request() behaving otherwise can
lead to bogus "duplicate lock owners detected" errors getting raised
during an attempt to handle any maintenance operation notification in
ImageWatcher::handle_operation_request().  This error isn't considered
retryable so the entire operation that needed the exclusive lock would
be spuriously failed with EINVAL.

Fixes: https://tracker.ceph.com/issues/74168
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc
index d2d99dec9a944572407e656bee21b704cc8d6a29..33061c09e7999d49561aac222c1a37333fcd3a8e 100644 (file)
--- a/src/librbd/ExclusiveLock.cc
+++ b/src/librbd/ExclusiveLock.cc
@@ -54,12 +54,21 @@ bool ExclusiveLock<I>::accept_request(OperationRequestType request_type,
                                       int *ret_val) const {
   std::lock_guard locker{ML<I>::m_lock};
 
-  bool accept_request =
-    (!ML<I>::is_state_shutdown() && ML<I>::is_state_locked() &&
-     (m_request_blocked_count == 0 ||
-      m_image_ctx.get_exclusive_lock_policy()->accept_blocked_request(
-        request_type)));
-  *ret_val = accept_request ? 0 : m_request_blocked_ret_val;
+  bool accept_request;
+  if (!ML<I>::is_state_shutdown() && ML<I>::is_state_locked()) {
+    if (m_request_blocked_count == 0 ||
+        m_image_ctx.get_exclusive_lock_policy()->accept_blocked_request(
+          request_type)) {
+      accept_request = true;
+      *ret_val = 0;
+    } else {
+      accept_request = false;
+      *ret_val = m_request_blocked_ret_val;
+    }
+  } else {
+    accept_request = false;
+    *ret_val = 0;
+  }
 
   ldout(m_image_ctx.cct, 20) << "=" << accept_request << " ret_val=" << *ret_val
                              << " (request_type=" << request_type << ")" << dendl;

diff --git a/src/test/librbd/test_mock_ExclusiveLock.cc b/src/test/librbd/test_mock_ExclusiveLock.cc
index 3638175fe30b3248c1abde91b504bf1dfe3f2784..64c3f7b5268a411de7a14da5acc4d04521c71b16 100644 (file)
--- a/src/test/librbd/test_mock_ExclusiveLock.cc
+++ b/src/test/librbd/test_mock_ExclusiveLock.cc
@@ -800,6 +800,12 @@ TEST_F(TestMockExclusiveLock, BlockRequests) {
                 exclusive_lock::OPERATION_REQUEST_TYPE_GENERAL, &ret_val));
   ASSERT_EQ(0, ret_val);
 
+  expect_is_state_shutdown(exclusive_lock, false);
+  expect_is_state_locked(exclusive_lock, false);
+  ASSERT_FALSE(exclusive_lock->accept_request(
+                 exclusive_lock::OPERATION_REQUEST_TYPE_GENERAL, &ret_val));
+  ASSERT_EQ(0, ret_val);
+
   exclusive_lock->block_requests(-EROFS);
   expect_is_state_shutdown(exclusive_lock, false);
   expect_is_state_locked(exclusive_lock, true);
@@ -810,6 +816,12 @@ TEST_F(TestMockExclusiveLock, BlockRequests) {
                  exclusive_lock::OPERATION_REQUEST_TYPE_GENERAL, &ret_val));
   ASSERT_EQ(-EROFS, ret_val);
 
+  expect_is_state_shutdown(exclusive_lock, false);
+  expect_is_state_locked(exclusive_lock, false);
+  ASSERT_FALSE(exclusive_lock->accept_request(
+                 exclusive_lock::OPERATION_REQUEST_TYPE_GENERAL, &ret_val));
+  ASSERT_EQ(0, ret_val);
+
   expect_is_state_shutdown(exclusive_lock, false);
   expect_is_state_locked(exclusive_lock, true);
   expect_accept_blocked_request(