From 110ecaf0607f3f3d33b47588177c31ba33d60051 Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Tue, 24 Jan 2023 21:51:34 -0800 Subject: [PATCH] crimson/osd/pg_backend: remove stopping and peering These two state variables duplicate checks that *should* already be handled by the IOInterruptCondition. None of the stopping checks should ever trigger because the caller would be in an interruptible future context which already performed that check. Moreover, peering doesn't really work -- it relies on the callback firing prior the call to on_activate_complete(), and there isn't any guarantee that will happen. Storing the epoch from when the callback was created as we do in IOInterruptCondition would be required. Signed-off-by: Samuel Just --- src/crimson/osd/ec_backend.h | 2 +- src/crimson/osd/pg.cc | 3 +-- src/crimson/osd/pg_backend.cc | 38 --------------------------- src/crimson/osd/pg_backend.h | 8 +----- src/crimson/osd/replicated_backend.cc | 24 +++++------------ src/crimson/osd/replicated_backend.h | 2 +- 6 files changed, 11 insertions(+), 66 deletions(-) diff --git a/src/crimson/osd/ec_backend.h b/src/crimson/osd/ec_backend.h index b9aeaf36f3bb8..74e3cd8919b84 100644 --- a/src/crimson/osd/ec_backend.h +++ b/src/crimson/osd/ec_backend.h @@ -21,7 +21,7 @@ public: seastar::future<> stop() final { return seastar::now(); } - void on_actingset_changed(peering_info_t pi) final {} + void on_actingset_changed(bool same_primary) final {} private: ll_read_ierrorator::future _read(const hobject_t& hoid, uint64_t off, uint64_t len, uint32_t flags) override; diff --git a/src/crimson/osd/pg.cc b/src/crimson/osd/pg.cc index 70851ece45ea9..53b392ab0e692 100644 --- a/src/crimson/osd/pg.cc +++ b/src/crimson/osd/pg.cc @@ -327,7 +327,6 @@ void PG::on_activate_complete() PeeringState::AllReplicasRecovered{}); } publish_stats_to_osd(); - backend->on_activate_complete(); } void PG::prepare_write(pg_info_t &info, @@ -1203,7 +1202,7 @@ void PG::on_change(ceph::os::Transaction &t) { logger().debug("{} {}:", *this, __func__); obc_loader.notify_on_change(is_primary()); recovery_backend->on_peering_interval_change(t); - backend->on_actingset_changed({ is_primary() }); + backend->on_actingset_changed(is_primary()); wait_for_active_blocker.unblock(); if (is_primary()) { logger().debug("{} {}: requeueing", *this, __func__); diff --git a/src/crimson/osd/pg_backend.cc b/src/crimson/osd/pg_backend.cc index 38607cb2f6ff0..86011bf081f63 100644 --- a/src/crimson/osd/pg_backend.cc +++ b/src/crimson/osd/pg_backend.cc @@ -79,10 +79,6 @@ PGBackend::load_metadata_iertr::future PGBackend::load_metadata(const hobject_t& oid) { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } - return interruptor::make_interruptible(store->get_attrs( coll, ghobject_t{oid, ghobject_t::NO_GEN, shard})).safe_then_interruptible( @@ -1003,10 +999,6 @@ PGBackend::remove(ObjectState& os, ceph::os::Transaction& txn, PGBackend::interruptible_future, hobject_t>> PGBackend::list_objects(const hobject_t& start, uint64_t limit) const { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } - auto gstart = start.is_min() ? ghobject_t{} : ghobject_t{start, 0, shard}; return interruptor::make_interruptible(store->list_objects(coll, gstart, @@ -1095,10 +1087,6 @@ PGBackend::getxattr( const hobject_t& soid, std::string_view key) const { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } - return store->get_attr(coll, ghobject_t{soid}, key); } @@ -1107,9 +1095,6 @@ PGBackend::getxattr( const hobject_t& soid, std::string&& key) const { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } return seastar::do_with(key, [this, &soid](auto &key) { return store->get_attr(coll, ghobject_t{soid}, key); }); @@ -1120,9 +1105,6 @@ PGBackend::get_attr_ierrorator::future<> PGBackend::get_xattrs( OSDOp& osd_op, object_stat_sum_t& delta_stats) const { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } return store->get_attrs(coll, ghobject_t{os.oi.soid}).safe_then( [&delta_stats, &osd_op](auto&& attrs) { std::vector> user_xattrs; @@ -1254,9 +1236,6 @@ PGBackend::rm_xattr( const OSDOp& osd_op, ceph::os::Transaction& txn) { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } if (!os.exists || os.oi.is_whiteout()) { logger().debug("{}: {} DNE", __func__, os.oi.soid); return crimson::ct_error::enoent::make(); @@ -1358,9 +1337,6 @@ PGBackend::omap_get_keys( OSDOp& osd_op, object_stat_sum_t& delta_stats) const { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } if (!os.exists || os.oi.is_whiteout()) { logger().debug("{}: object does not exist: {}", os.oi.soid); return crimson::ct_error::enoent::make(); @@ -1483,10 +1459,6 @@ PGBackend::omap_get_vals( OSDOp& osd_op, object_stat_sum_t& delta_stats) const { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } - if (!os.exists || os.oi.is_whiteout()) { logger().debug("{}: object does not exist: {}", os.oi.soid); return crimson::ct_error::enoent::make(); @@ -1553,9 +1525,6 @@ PGBackend::omap_get_vals_by_keys( OSDOp& osd_op, object_stat_sum_t& delta_stats) const { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } if (!os.exists || os.oi.is_whiteout()) { logger().debug("{}: object does not exist: {}", __func__, os.oi.soid); return crimson::ct_error::enoent::make(); @@ -1678,9 +1647,6 @@ PGBackend::omap_clear( osd_op_params_t& osd_op_params, object_stat_sum_t& delta_stats) { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } if (!os.exists || os.oi.is_whiteout()) { logger().debug("{}: object does not exist: {}", os.oi.soid); return crimson::ct_error::enoent::make(); @@ -1821,7 +1787,3 @@ PGBackend::read_ierrorator::future<> PGBackend::tmapget( read_errorator::pass_further{}); } -void PGBackend::on_activate_complete() { - peering.reset(); -} - diff --git a/src/crimson/osd/pg_backend.h b/src/crimson/osd/pg_backend.h index b833c8403f229..aa581134420ae 100644 --- a/src/crimson/osd/pg_backend.h +++ b/src/crimson/osd/pg_backend.h @@ -383,19 +383,13 @@ public: virtual void got_rep_op_reply(const MOSDRepOpReply&) {} virtual seastar::future<> stop() = 0; - struct peering_info_t { - bool is_primary; - }; - virtual void on_actingset_changed(peering_info_t pi) = 0; - virtual void on_activate_complete(); + virtual void on_actingset_changed(bool same_primary) = 0; protected: const shard_id_t shard; CollectionRef coll; crimson::osd::ShardServices &shard_services; DoutPrefixProvider &dpp; ///< provides log prefix context crimson::os::FuturizedStore* store; - bool stopping = false; - std::optional peering; virtual seastar::future<> request_committed( const osd_reqid_t& reqid, const eversion_t& at_version) = 0; diff --git a/src/crimson/osd/replicated_backend.cc b/src/crimson/osd/replicated_backend.cc index 5693a2f30aaab..8db67d356bbd9 100644 --- a/src/crimson/osd/replicated_backend.cc +++ b/src/crimson/osd/replicated_backend.cc @@ -29,9 +29,6 @@ ReplicatedBackend::_read(const hobject_t& hoid, const uint64_t len, const uint32_t flags) { - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } return store->read(coll, ghobject_t{hoid}, off, len, flags); } @@ -44,12 +41,6 @@ ReplicatedBackend::_submit_transaction(std::set&& pg_shards, std::vector&& log_entries) { LOG_PREFIX(ReplicatedBackend::_submit_transaction); - if (__builtin_expect(stopping, false)) { - throw crimson::common::system_shutdown_exception(); - } - if (__builtin_expect((bool)peering, false)) { - throw crimson::common::actingset_changed(peering->is_primary); - } const ceph_tid_t tid = shard_services.get_tid(); auto pending_txn = @@ -59,13 +50,14 @@ ReplicatedBackend::_submit_transaction(std::set&& pg_shards, DEBUGDPP("object {}", dpp, hoid); auto all_completed = interruptor::make_interruptible( - shard_services.get_store().do_transaction(coll, std::move(txn))) - .then_interruptible([this, peers=pending_txn->second.weak_from_this()] { + shard_services.get_store().do_transaction(coll, std::move(txn)) + ).then_interruptible([FNAME, this, + peers=pending_txn->second.weak_from_this()] { if (!peers) { // for now, only actingset_changed can cause peers // to be nullptr - assert(peering); - throw crimson::common::actingset_changed(peering->is_primary); + ERRORDPP("peers is null, this should be impossible", dpp); + assert(0 == "impossible"); } if (--peers->pending == 0) { peers->all_committed.set_value(); @@ -108,10 +100,9 @@ ReplicatedBackend::_submit_transaction(std::set&& pg_shards, return {std::move(sends_complete), std::move(all_completed)}; } -void ReplicatedBackend::on_actingset_changed(peering_info_t pi) +void ReplicatedBackend::on_actingset_changed(bool same_primary) { - peering.emplace(pi); - crimson::common::actingset_changed e_actingset_changed{peering->is_primary}; + crimson::common::actingset_changed e_actingset_changed{same_primary}; for (auto& [tid, pending_txn] : pending_trans) { pending_txn.all_committed.set_exception(e_actingset_changed); } @@ -143,7 +134,6 @@ seastar::future<> ReplicatedBackend::stop() { LOG_PREFIX(ReplicatedBackend::stop); INFODPP("cid {}", coll->get_cid()); - stopping = true; for (auto& [tid, pending_on] : pending_trans) { pending_on.all_committed.set_exception( crimson::common::system_shutdown_exception()); diff --git a/src/crimson/osd/replicated_backend.h b/src/crimson/osd/replicated_backend.h index eab0d4d5f3ddc..f789a35eae690 100644 --- a/src/crimson/osd/replicated_backend.h +++ b/src/crimson/osd/replicated_backend.h @@ -25,7 +25,7 @@ public: DoutPrefixProvider &dpp); void got_rep_op_reply(const MOSDRepOpReply& reply) final; seastar::future<> stop() final; - void on_actingset_changed(peering_info_t pi) final; + void on_actingset_changed(bool same_primary) final; private: ll_read_ierrorator::future _read(const hobject_t& hoid, uint64_t off, -- 2.39.5