From 46f01d832487d3a3183783d41450fd8f49347097 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 --- 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 3a3a4a13729..cbe89594118 100644 --- a/src/common/options/global.yaml.in +++ b/src/common/options/global.yaml.in @@ -6343,3 +6343,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 e3e4833f1a2..3d9f680ba6c 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -7852,6 +7852,7 @@ int BlueStore::_mount() int BlueStore::umount() { + dout(5) << __func__ << dendl; ceph_assert(_kv_only || mounted); _osr_drain_all(); @@ -14434,7 +14435,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 @@ -14680,7 +14687,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 c3c53264ec1..486c7cd4de0 100644 --- a/src/os/bluestore/BlueStore.h +++ b/src/os/bluestore/BlueStore.h @@ -2055,6 +2055,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 9e3c7acd73b..30a7606fd2f 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 99e99dcba04..8ada7524dbc 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 482d3028304..d23c17a019d 100644 --- a/src/test/objectstore/store_test.cc +++ b/src/test/objectstore/store_test.cc @@ -3245,7 +3245,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); { @@ -3531,8 +3532,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; @@ -3554,8 +3559,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.39.5