From 2f35e2d840c090552b963ef85e5a0fe9d09c9ed4 Mon Sep 17 00:00:00 2001 From: Igor Fedotov Date: Fri, 25 Aug 2023 16:10:26 +0300 Subject: [PATCH] test/store_test: get rid off assert_death. Looks like death assertions aren't 100% reliable and might cause deadlock sometimes. Hence getting rid of them and enabling optional sending exception from *Store::queue_transaction() Fixes: https://tracker.ceph.com/issues/61193 Signed-off-by: Igor Fedotov (cherry picked from commit 46f01d832487d3a3183783d41450fd8f49347097) --- src/common/options/global.yaml.in | 6 ++++++ src/os/bluestore/BlueStore.cc | 17 +++++++++++++++-- src/os/bluestore/BlueStore.h | 6 ++++++ src/os/kstore/KStore.cc | 16 ++++++++++++++-- src/os/kstore/KStore.h | 5 +++++ src/os/memstore/MemStore.cc | 6 +++++- src/test/objectstore/store_test.cc | 19 ++++++++++++++----- src/test/objectstore/store_test_fixture.cc | 4 ---- src/test/objectstore/store_test_fixture.h | 8 -------- 9 files changed, 65 insertions(+), 22 deletions(-) diff --git a/src/common/options/global.yaml.in b/src/common/options/global.yaml.in index a0ebf3576c9..b3c5130c400 100644 --- a/src/common/options/global.yaml.in +++ b/src/common/options/global.yaml.in @@ -6360,3 +6360,9 @@ options: default: 0 services: - mgr +- name: objectstore_debug_throw_on_failed_txc + type: bool + level: dev + desc: Enables exception throwing instead of process abort on transaction submission error. + default: false + with_legacy: false diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index ebf70f76e68..07386063db5 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -7787,6 +7787,7 @@ int BlueStore::_mount() int BlueStore::umount() { + dout(5) << __func__ << dendl; ceph_assert(_kv_only || mounted); _osr_drain_all(); @@ -14366,7 +14367,13 @@ void BlueStore::_txc_add_transaction(TransContext *txc, Transaction *t) << " not handled on operation " << op->op << " (op " << pos << ", counting from 0)" << dendl; _dump_transaction<0>(cct, t); - ceph_abort_msg("unexpected error"); + if (!g_conf().get_val("objectstore_debug_throw_on_failed_txc")) { + ceph_abort_msg("unexpected error"); + } else { + txc->osr->undo_queue(txc); + delete txc; + throw r; + } } // these operations implicity create the object @@ -14612,7 +14619,13 @@ void BlueStore::_txc_add_transaction(TransContext *txc, Transaction *t) << dendl; derr << msg << dendl; _dump_transaction<0>(cct, t); - ceph_abort_msg("unexpected error"); + if (!g_conf().get_val("objectstore_debug_throw_on_failed_txc")) { + ceph_abort_msg("unexpected error"); + } else { + txc->osr->undo_queue(txc); + delete txc; + throw r; + } } } } diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h index 02a3bd51c87..a91bf8414eb 100644 --- a/src/os/bluestore/BlueStore.h +++ b/src/os/bluestore/BlueStore.h @@ -2045,6 +2045,12 @@ private: txc->seq = ++last_seq; q.push_back(*txc); } + void undo_queue(TransContext* txc) { + std::lock_guard l(qlock); + ceph_assert(&q.back() == txc); + --last_seq; + q.pop_back(); + } void drain() { std::unique_lock l(qlock); diff --git a/src/os/kstore/KStore.cc b/src/os/kstore/KStore.cc index 9526a756419..7158486ca38 100644 --- a/src/os/kstore/KStore.cc +++ b/src/os/kstore/KStore.cc @@ -2285,7 +2285,13 @@ void KStore::_txc_add_transaction(TransContext *txc, Transaction *t) f.close_section(); f.flush(*_dout); *_dout << dendl; - ceph_abort_msg("unexpected error"); + if (!g_conf().get_val("objectstore_debug_throw_on_failed_txc")) { + ceph_abort_msg("unexpected error"); + } else { + txc->osr->undo_queue(txc); + delete txc; + throw r; + } } // object operations @@ -2534,7 +2540,13 @@ void KStore::_txc_add_transaction(TransContext *txc, Transaction *t) f.close_section(); f.flush(*_dout); *_dout << dendl; - ceph_abort_msg("unexpected error"); + if (!g_conf().get_val("objectstore_debug_throw_on_failed_txc")) { + ceph_abort_msg("unexpected error"); + } else { + txc->osr->undo_queue(txc); + delete txc; + throw r; + } } } } diff --git a/src/os/kstore/KStore.h b/src/os/kstore/KStore.h index d7d95acdfbd..808543cefde 100644 --- a/src/os/kstore/KStore.h +++ b/src/os/kstore/KStore.h @@ -276,6 +276,11 @@ public: std::lock_guard l(qlock); q.push_back(*txc); } + void undo_queue(TransContext* txc) { + std::lock_guard l(qlock); + ceph_assert(&q.back() == txc); + q.pop_back(); + } void flush() { std::unique_lock l(qlock); diff --git a/src/os/memstore/MemStore.cc b/src/os/memstore/MemStore.cc index e0a607270d1..4f705afccf0 100644 --- a/src/os/memstore/MemStore.cc +++ b/src/os/memstore/MemStore.cc @@ -1032,7 +1032,11 @@ void MemStore::_do_transaction(Transaction& t) f.close_section(); f.flush(*_dout); *_dout << dendl; - ceph_abort_msg("unexpected error"); + if (!g_conf().get_val("objectstore_debug_throw_on_failed_txc")) { + ceph_abort_msg("unexpected error"); + } else { + throw r; + } } } diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc index 54dc29c7db8..96f6aff4c7b 100644 --- a/src/test/objectstore/store_test.cc +++ b/src/test/objectstore/store_test.cc @@ -3246,7 +3246,8 @@ TEST_P(StoreTest, SimpleCloneTest) { int r; coll_t cid; - SetDeathTestStyle("threadsafe"); + SetVal(g_conf(), "objectstore_debug_throw_on_failed_txc", "true"); + g_conf().apply_changes(nullptr); auto ch = store->create_new_collection(cid); { @@ -3532,8 +3533,12 @@ TEST_P(StoreTest, SimpleCloneTest) { ObjectStore::Transaction t; t.remove_collection(cid); cerr << "Invalid rm coll" << std::endl; - PrCtl unset_dumpable; - EXPECT_DEATH(queue_transaction(store, ch, std::move(t)), ""); + try { + queue_transaction(store, ch, std::move(t)); + FAIL() << "remove_collection failed to return ENOTEMPTY."; + } catch (int err) { + ASSERT_EQ(err, -ENOTEMPTY); + } } { ObjectStore::Transaction t; @@ -3555,8 +3560,12 @@ TEST_P(StoreTest, SimpleCloneTest) { t.remove(cid, hoid); t.remove(cid, hoid2); t.remove_collection(cid); - PrCtl unset_dumpable; - EXPECT_DEATH(queue_transaction(store, ch, std::move(t)), ""); + try { + queue_transaction(store, ch, std::move(t)); + FAIL() << "remove_collection failed to return ENOTEMPTY."; + } catch (int err) { + ASSERT_EQ(err, -ENOTEMPTY); + } } { ObjectStore::Transaction t; diff --git a/src/test/objectstore/store_test_fixture.cc b/src/test/objectstore/store_test_fixture.cc index a3bdc7a36ac..0cffd79a709 100644 --- a/src/test/objectstore/store_test_fixture.cc +++ b/src/test/objectstore/store_test_fixture.cc @@ -77,10 +77,6 @@ void StoreTestFixture::TearDown() // config settings. Hence setting it to 'unsafe' here as test case is closing. g_conf()._clear_safe_to_start_threads(); PopSettings(0); - if (!orig_death_test_style.empty()) { - ::testing::FLAGS_gtest_death_test_style = orig_death_test_style; - orig_death_test_style.clear(); - } } void StoreTestFixture::SetVal(ConfigProxy& _conf, const char* key, const char* val) diff --git a/src/test/objectstore/store_test_fixture.h b/src/test/objectstore/store_test_fixture.h index 3f25fd493d0..0495c21bd32 100644 --- a/src/test/objectstore/store_test_fixture.h +++ b/src/test/objectstore/store_test_fixture.h @@ -13,8 +13,6 @@ class StoreTestFixture : virtual public ::testing::Test { std::stack> saved_settings; ConfigProxy* conf = nullptr; - std::string orig_death_test_style; - public: std::unique_ptr store; ObjectStore::CollectionHandle ch; @@ -25,12 +23,6 @@ public: void SetUp() override; void TearDown() override; - void SetDeathTestStyle(const char* new_style) { - if (orig_death_test_style.empty()) { - orig_death_test_style = ::testing::FLAGS_gtest_death_test_style; - } - ::testing::FLAGS_gtest_death_test_style = new_style; - } void SetVal(ConfigProxy& conf, const char* key, const char* val); struct SettingsBookmark { -- 2.47.3