From: Ilya Dryomov Date: Tue, 28 Mar 2023 18:03:05 +0000 (+0200) Subject: librbd: avoid generating ESHUTDOWN in ManagedLock X-Git-Tag: v19.0.0~1418^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=76856d90e0e0d73276c06a724091197301f982e9;p=ceph-ci.git 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 --- 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)); }