From: Samuel Just Date: Wed, 25 Jan 2023 05:51:34 +0000 (-0800) Subject: crimson/osd/pg_backend: remove stopping and peering X-Git-Tag: v18.1.0~419^2~5 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=110ecaf0607f3f3d33b47588177c31ba33d60051;p=ceph.git 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 --- diff --git a/src/crimson/osd/ec_backend.h b/src/crimson/osd/ec_backend.h index b9aeaf36f3bb..74e3cd8919b8 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 70851ece45ea..53b392ab0e69 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 38607cb2f6ff..86011bf081f6 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 b833c8403f22..aa581134420a 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 5693a2f30aaa..8db67d356bbd 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 eab0d4d5f3dd..f789a35eae69 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,