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 <sjust@redhat.com>

diff --git a/src/crimson/osd/ec_backend.h b/src/crimson/osd/ec_backend.h
index b9aeaf36f3bb8bba13de319ad4cec3ef93ebdebb..74e3cd8919b84a9cf9d69cce18f144572a13d397 100644 (file)
--- 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<ceph::bufferlist>
   _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 70851ece45ea97c0f6abf3633604f352cb21a170..53b392ab0e69264e20ec9796437a89c1488f47d9 100644 (file)
--- 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 38607cb2f6ff0b2c5971e8a2571b03772ce70ec9..86011bf081f63b1aa36bf8718853b94a35fe6e1d 100644 (file)
--- a/src/crimson/osd/pg_backend.cc
+++ b/src/crimson/osd/pg_backend.cc
@@ -79,10 +79,6 @@ PGBackend::load_metadata_iertr::future
   <PGBackend::loaded_object_md_t::ref>
 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<std::tuple<std::vector<hobject_t>, 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<std::pair<std::string, bufferlist>> 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 b833c8403f229dba07c6fb113163f060d69286e7..aa581134420aeebe70533b19da7a7173bbb769b4 100644 (file)
--- 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_info_t> 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 5693a2f30aaab5de3301526e225bfee8d8a254b3..8db67d356bbd99d70a3999bda71a2283eb8e585a 100644 (file)
--- 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_shard_t>&& pg_shards,
                                       std::vector<pg_log_entry_t>&& 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_shard_t>&& 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_shard_t>&& 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 eab0d4d5f3ddc16daf2381e9ae7d7793b8d547ab..f789a35eae690ec9a05d1026286a9563e0ba86d4 100644 (file)
--- 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<ceph::bufferlist>
     _read(const hobject_t& hoid, uint64_t off,