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>
(cherry picked from commit e4855895a9f14a07fda03fbc736f596b87f92327)

diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc
index fc2d2186ea828238f8c7a018eee8da891e5ed061..8c185a421da8351e9a8b9af4ee6b63de861fb972 100644 (file)
--- a/src/librbd/ExclusiveLock.cc
+++ b/src/librbd/ExclusiveLock.cc
@@ -52,12 +52,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 6feb54ec66146c1f5a910a36e3bc4b9fd19e73d5..e9fbae2e4d9fba2ed7ec79b8411712f191cef0ae 100644 (file)
--- a/src/test/librbd/test_mock_ExclusiveLock.cc
+++ b/src/test/librbd/test_mock_ExclusiveLock.cc
@@ -799,6 +799,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);
@@ -809,6 +815,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(