From 2ddb228504b28af31004c13cb21971447794979e Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Tue, 28 Mar 2023 20:03:05 +0200 Subject: [PATCH] librbd: avoid generating ESHUTDOWN in ManagedLock EBLOCKLISTED has a very special meaning but happens to be an alias for ESHUTDOWN. If the client gets blocklisted, we always want to propagate EBLOCKLISTED error code since it's generated by the OSD. For ManagedLock use case of indicating that an operation on the lock raced with lock shut down, meaning that a higher level request can just be restarted, ERESTART should do. Signed-off-by: Ilya Dryomov (cherry picked from commit 76856d90e0e0d73276c06a724091197301f982e9) --- src/librbd/ManagedLock.cc | 14 ++--- src/librbd/exclusive_lock/ImageDispatch.cc | 2 +- src/test/librbd/test_internal.cc | 72 ++++++++++++++++++++++ src/test/librbd/test_mock_ManagedLock.cc | 2 +- 4 files changed, 81 insertions(+), 9 deletions(-) diff --git a/src/librbd/ManagedLock.cc b/src/librbd/ManagedLock.cc index 7b785a261a3..bb11160cb54 100644 --- a/src/librbd/ManagedLock.cc +++ b/src/librbd/ManagedLock.cc @@ -139,7 +139,7 @@ void ManagedLock::shut_down(Context *on_shut_down) { Action active_action = get_active_action(); ceph_assert(active_action == ACTION_TRY_LOCK || active_action == ACTION_ACQUIRE_LOCK); - complete_active_action(STATE_UNLOCKED, -ESHUTDOWN); + complete_active_action(STATE_UNLOCKED, -ERESTART); } execute_action(ACTION_SHUT_DOWN, on_shut_down); @@ -151,7 +151,7 @@ void ManagedLock::acquire_lock(Context *on_acquired) { { std::lock_guard locker{m_lock}; if (is_state_shutdown()) { - r = -ESHUTDOWN; + r = -ERESTART; } else if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) { ldout(m_cct, 10) << dendl; execute_action(ACTION_ACQUIRE_LOCK, on_acquired); @@ -170,7 +170,7 @@ void ManagedLock::try_acquire_lock(Context *on_acquired) { { std::lock_guard locker{m_lock}; if (is_state_shutdown()) { - r = -ESHUTDOWN; + r = -ERESTART; } else if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) { ldout(m_cct, 10) << dendl; execute_action(ACTION_TRY_LOCK, on_acquired); @@ -189,7 +189,7 @@ void ManagedLock::release_lock(Context *on_released) { { std::lock_guard locker{m_lock}; if (is_state_shutdown()) { - r = -ESHUTDOWN; + r = -ERESTART; } else if (m_state != STATE_UNLOCKED || !m_actions_contexts.empty()) { ldout(m_cct, 10) << dendl; execute_action(ACTION_RELEASE_LOCK, on_released); @@ -241,7 +241,7 @@ void ManagedLock::get_locker(managed_lock::Locker *locker, { std::lock_guard l{m_lock}; if (is_state_shutdown()) { - r = -ESHUTDOWN; + r = -ERESTART; } else { on_finish = new C_Tracked(m_async_op_tracker, on_finish); auto req = managed_lock::GetLockerRequest::create( @@ -263,7 +263,7 @@ void ManagedLock::break_lock(const managed_lock::Locker &locker, { std::lock_guard l{m_lock}; if (is_state_shutdown()) { - r = -ESHUTDOWN; + r = -ERESTART; } else if (is_lock_owner(m_lock)) { r = -EBUSY; } else { @@ -490,7 +490,7 @@ void ManagedLock::send_acquire_lock() { // shut down might race w/ release/re-acquire of the lock if (is_state_shutdown()) { - complete_active_action(STATE_UNLOCKED, -ESHUTDOWN); + complete_active_action(STATE_UNLOCKED, -ERESTART); } } return; diff --git a/src/librbd/exclusive_lock/ImageDispatch.cc b/src/librbd/exclusive_lock/ImageDispatch.cc index 6b403e2e315..f53b3692a1b 100644 --- a/src/librbd/exclusive_lock/ImageDispatch.cc +++ b/src/librbd/exclusive_lock/ImageDispatch.cc @@ -293,7 +293,7 @@ void ImageDispatch::handle_acquire_lock(int r) { Context* failed_dispatch = nullptr; Contexts on_dispatches; - if (r == -ESHUTDOWN) { + if (r == -ERESTART) { ldout(cct, 5) << "IO raced with exclusive lock shutdown" << dendl; } else if (r < 0) { lderr(cct) << "failed to acquire exclusive lock: " << cpp_strerror(r) diff --git a/src/test/librbd/test_internal.cc b/src/test/librbd/test_internal.cc index 06423d4f105..0ec4462687d 100644 --- a/src/test/librbd/test_internal.cc +++ b/src/test/librbd/test_internal.cc @@ -4,6 +4,7 @@ #include "cls/journal/cls_journal_client.h" #include "cls/rbd/cls_rbd_client.h" #include "cls/rbd/cls_rbd_types.h" +#include "test/librados/test_cxx.h" #include "test/librbd/test_fixture.h" #include "test/librbd/test_support.h" #include "include/rbd/librbd.h" @@ -376,6 +377,77 @@ TEST_F(TestInternal, FlattenFailsToLockImage) { ASSERT_EQ(-EROFS, ictx2->operations->flatten(no_op)); } +TEST_F(TestInternal, WriteFailsToLockImageBlocklisted) { + REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); + + librados::Rados blocklist_rados; + ASSERT_EQ("", connect_cluster_pp(blocklist_rados)); + + librados::IoCtx blocklist_ioctx; + ASSERT_EQ(0, blocklist_rados.ioctx_create(_pool_name.c_str(), + blocklist_ioctx)); + + auto ictx = new librbd::ImageCtx(m_image_name, "", nullptr, blocklist_ioctx, + false); + ASSERT_EQ(0, ictx->state->open(0)); + + std::list watchers; + ASSERT_EQ(0, librbd::list_watchers(ictx, watchers)); + ASSERT_EQ(1U, watchers.size()); + + bool lock_owner; + ASSERT_EQ(0, librbd::is_exclusive_lock_owner(ictx, &lock_owner)); + ASSERT_FALSE(lock_owner); + + ASSERT_EQ(0, blocklist_rados.blocklist_add(watchers.front().addr, 0)); + + ceph::bufferlist bl; + bl.append(std::string(256, '1')); + ASSERT_EQ(-EBLOCKLISTED, api::Io<>::write(*ictx, 0, bl.length(), + std::move(bl), 0)); + ASSERT_EQ(-EBLOCKLISTED, librbd::is_exclusive_lock_owner(ictx, &lock_owner)); + + close_image(ictx); +} + +TEST_F(TestInternal, WriteFailsToLockImageBlocklistedWatch) { + REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); + + librados::Rados blocklist_rados; + ASSERT_EQ("", connect_cluster_pp(blocklist_rados)); + + librados::IoCtx blocklist_ioctx; + ASSERT_EQ(0, blocklist_rados.ioctx_create(_pool_name.c_str(), + blocklist_ioctx)); + + auto ictx = new librbd::ImageCtx(m_image_name, "", nullptr, blocklist_ioctx, + false); + ASSERT_EQ(0, ictx->state->open(0)); + + std::list watchers; + ASSERT_EQ(0, librbd::list_watchers(ictx, watchers)); + ASSERT_EQ(1U, watchers.size()); + + bool lock_owner; + ASSERT_EQ(0, librbd::is_exclusive_lock_owner(ictx, &lock_owner)); + ASSERT_FALSE(lock_owner); + + ASSERT_EQ(0, blocklist_rados.blocklist_add(watchers.front().addr, 0)); + // let ImageWatcher discover that the watch can't be re-registered to + // eliminate the (intended) race in WriteFailsToLockImageBlocklisted + while (!ictx->image_watcher->is_blocklisted()) { + sleep(1); + } + + ceph::bufferlist bl; + bl.append(std::string(256, '1')); + ASSERT_EQ(-EBLOCKLISTED, api::Io<>::write(*ictx, 0, bl.length(), + std::move(bl), 0)); + ASSERT_EQ(-EBLOCKLISTED, librbd::is_exclusive_lock_owner(ictx, &lock_owner)); + + close_image(ictx); +} + TEST_F(TestInternal, AioWriteRequestsLock) { REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); diff --git a/src/test/librbd/test_mock_ManagedLock.cc b/src/test/librbd/test_mock_ManagedLock.cc index dfa0f84fe10..e3d060771ff 100644 --- a/src/test/librbd/test_mock_ManagedLock.cc +++ b/src/test/librbd/test_mock_ManagedLock.cc @@ -716,7 +716,7 @@ TEST_F(TestMockManagedLock, ShutDownWhileWaiting) { managed_lock.acquire_lock(&acquire_ctx); ASSERT_EQ(0, when_shut_down(managed_lock)); - ASSERT_EQ(-ESHUTDOWN, acquire_ctx.wait()); + ASSERT_EQ(-ERESTART, acquire_ctx.wait()); ASSERT_FALSE(is_lock_owner(managed_lock)); } -- 2.39.5