Merge pull request ceph#59407 from xxhdx1985126/wip-67675

crimson/osd/pg: only mutate obc for user triggerred transactions

Reviewed-by: Matan Breizman <[email protected]>
Reviewed-by: Samuel Just <[email protected]>

Author: Matan-B

src/crimson/osd/ec_backend.cc

@@ -24,12 +24,12 @@ ECBackend::_read(const hobject_t& hoid,
 }
 
 ECBackend::rep_op_fut_t
-ECBackend::_submit_transaction(std::set<pg_shard_t>&& pg_shards,
-                               const hobject_t& hoid,
-                               ceph::os::Transaction&& txn,
-                               osd_op_params_t&& osd_op_p,
-                               epoch_t min_epoch, epoch_t max_epoch,
-			       std::vector<pg_log_entry_t>&& log_entries)
+ECBackend::submit_transaction(const std::set<pg_shard_t> &pg_shards,
+                              const hobject_t& hoid,
+                              ceph::os::Transaction&& txn,
+                              osd_op_params_t&& osd_op_p,
+                              epoch_t min_epoch, epoch_t max_epoch,
+			      std::vector<pg_log_entry_t>&& log_entries)
 {
   // todo
   return {seastar::now(),

src/crimson/osd/ec_backend.h

@@ -26,12 +26,12 @@ class ECBackend : public PGBackend
   ll_read_ierrorator::future<ceph::bufferlist>
   _read(const hobject_t& hoid, uint64_t off, uint64_t len, uint32_t flags) override;
   rep_op_fut_t
-  _submit_transaction(std::set<pg_shard_t>&& pg_shards,
-		      const hobject_t& hoid,
-		      ceph::os::Transaction&& txn,
-		      osd_op_params_t&& req,
-		      epoch_t min_epoch, epoch_t max_epoch,
-		      std::vector<pg_log_entry_t>&& log_entries) final;
+  submit_transaction(const std::set<pg_shard_t> &pg_shards,
+		     const hobject_t& hoid,
+		     ceph::os::Transaction&& txn,
+		     osd_op_params_t&& req,
+		     epoch_t min_epoch, epoch_t max_epoch,
+		     std::vector<pg_log_entry_t>&& log_entries) final;
   CollectionRef coll;
   seastar::future<> request_committed(const osd_reqid_t& reqid,
 				       const eversion_t& version) final {

src/crimson/osd/osd_operations/snaptrim_event.cc

@@ -255,7 +255,7 @@ SnapTrimObjSubEvent::adjust_snaps(
   obc->obs.oi.prior_version = obc->obs.oi.version;
   obc->obs.oi.version = osd_op_p.at_version;
   ceph::bufferlist bl;
-  encode(obc->obs.oi,
+  obc->obs.oi.encode_no_oid(
     bl,
     pg->get_osdmap()->get_features(CEPH_ENTITY_TYPE_OSD, nullptr));
   txn.setattr(

src/crimson/osd/pg.cc

@@ -854,6 +854,45 @@ std::ostream& operator<<(std::ostream& os, const PG& pg)
   return os;
 }
 
+void PG::mutate_object(
+  ObjectContextRef& obc,
+  ceph::os::Transaction& txn,
+  osd_op_params_t& osd_op_p)
+{
+  if (obc->obs.exists) {
+    obc->obs.oi.prior_version = obc->obs.oi.version;
+    obc->obs.oi.version = osd_op_p.at_version;
+    if (osd_op_p.user_modify)
+      obc->obs.oi.user_version = osd_op_p.at_version.version;
+    obc->obs.oi.last_reqid = osd_op_p.req_id;
+    obc->obs.oi.mtime = osd_op_p.mtime;
+    obc->obs.oi.local_mtime = ceph_clock_now();
+
+    // object_info_t
+    {
+      ceph::bufferlist osv;
+      obc->obs.oi.encode_no_oid(osv, CEPH_FEATURES_ALL);
+      // TODO: get_osdmap()->get_features(CEPH_ENTITY_TYPE_OSD, nullptr));
+      txn.setattr(coll_ref->get_cid(), ghobject_t{obc->obs.oi.soid}, OI_ATTR, osv);
+    }
+
+    // snapset
+    if (obc->obs.oi.soid.snap == CEPH_NOSNAP) {
+      logger().debug("final snapset {} in {}",
+        obc->ssc->snapset, obc->obs.oi.soid);
+      ceph::bufferlist bss;
+      encode(obc->ssc->snapset, bss);
+      txn.setattr(coll_ref->get_cid(), ghobject_t{obc->obs.oi.soid}, SS_ATTR, bss);
+      obc->ssc->exists = true;
+    } else {
+      logger().debug("no snapset (this is a clone)");
+    }
+  } else {
+    // reset cached ObjectState without enforcing eviction
+    obc->obs.oi = object_info_t(obc->obs.oi.soid);
+  }
+}
+
 std::tuple<PG::interruptible_future<>,
            PG::interruptible_future<>>
 PG::submit_transaction(
@@ -877,9 +916,9 @@ PG::submit_transaction(
   ceph_assert(log_entries.rbegin()->version >= projected_last_update);
   projected_last_update = log_entries.rbegin()->version;
 
-  auto [submitted, all_completed] = backend->mutate_object(
+  auto [submitted, all_completed] = backend->submit_transaction(
       peering_state.get_acting_recovery_backfill(),
-      std::move(obc),
+      obc->obs.oi.soid,
       std::move(txn),
       std::move(osd_op_p),
       peering_state.get_last_peering_reset(),
@@ -986,6 +1025,7 @@ PG::do_osd_ops_execute(
 	logger().debug(
 	  "do_osd_ops_execute: object {} submitting txn",
 	  obc->get_oid());
+        mutate_object(obc, txn, osd_op_p);
 	return submit_transaction(
           std::move(obc),
           std::move(txn),

src/crimson/osd/pg.h

@@ -863,6 +863,10 @@ class PG : public boost::intrusive_ref_counter<
   friend class SnapTrimObjSubEvent;
 private:
 
+  void mutate_object(
+    ObjectContextRef& obc,
+    ceph::os::Transaction& txn,
+    osd_op_params_t& osd_op_p);
   bool can_discard_replica_op(const Message& m, epoch_t m_map_epoch) const;
   bool can_discard_op(const MOSDOp& m) const;
   void context_registry_on_change();

src/crimson/osd/pg_backend.cc

@@ -152,54 +152,6 @@ PGBackend::load_metadata(const hobject_t& oid)
       }));
 }
 
-PGBackend::rep_op_fut_t
-PGBackend::mutate_object(
-  std::set<pg_shard_t> pg_shards,
-  crimson::osd::ObjectContextRef &&obc,
-  ceph::os::Transaction&& txn,
-  osd_op_params_t&& osd_op_p,
-  epoch_t min_epoch,
-  epoch_t map_epoch,
-  std::vector<pg_log_entry_t>&& log_entries)
-{
-  logger().trace("mutate_object: num_ops={}", txn.get_num_ops());
-  if (obc->obs.exists) {
-    obc->obs.oi.prior_version = obc->obs.oi.version;
-    obc->obs.oi.version = osd_op_p.at_version;
-    if (osd_op_p.user_modify)
-      obc->obs.oi.user_version = osd_op_p.at_version.version;
-    obc->obs.oi.last_reqid = osd_op_p.req_id;
-    obc->obs.oi.mtime = osd_op_p.mtime;
-    obc->obs.oi.local_mtime = ceph_clock_now();
-
-    // object_info_t
-    {
-      ceph::bufferlist osv;
-      obc->obs.oi.encode_no_oid(osv, CEPH_FEATURES_ALL);
-      // TODO: get_osdmap()->get_features(CEPH_ENTITY_TYPE_OSD, nullptr));
-      txn.setattr(coll->get_cid(), ghobject_t{obc->obs.oi.soid}, OI_ATTR, osv);
-    }
-
-    // snapset
-    if (obc->obs.oi.soid.snap == CEPH_NOSNAP) {
-      logger().debug("final snapset {} in {}",
-        obc->ssc->snapset, obc->obs.oi.soid);
-      ceph::bufferlist bss;
-      encode(obc->ssc->snapset, bss);
-      txn.setattr(coll->get_cid(), ghobject_t{obc->obs.oi.soid}, SS_ATTR, bss);
-      obc->ssc->exists = true;
-    } else {
-      logger().debug("no snapset (this is a clone)");
-    }
-  } else {
-    // reset cached ObjectState without enforcing eviction
-    obc->obs.oi = object_info_t(obc->obs.oi.soid);
-  }
-  return _submit_transaction(
-    std::move(pg_shards), obc->obs.oi.soid, std::move(txn),
-    std::move(osd_op_p), min_epoch, map_epoch, std::move(log_entries));
-}
-
 static inline bool _read_verify_data(
   const object_info_t& oi,
   const ceph::bufferlist& data)

src/crimson/osd/pg_backend.h

@@ -220,14 +220,6 @@ class PGBackend
     ceph::os::Transaction& trans,
     osd_op_params_t& osd_op_params,
     object_stat_sum_t& delta_stats);
-  rep_op_fut_t mutate_object(
-    std::set<pg_shard_t> pg_shards,
-    crimson::osd::ObjectContextRef &&obc,
-    ceph::os::Transaction&& txn,
-    osd_op_params_t&& osd_op_p,
-    epoch_t min_epoch,
-    epoch_t map_epoch,
-    std::vector<pg_log_entry_t>&& log_entries);
 
   /**
    * list_objects
@@ -418,6 +410,13 @@ class PGBackend
     ceph::os::Transaction& trans,
     osd_op_params_t& osd_op_params,
     object_stat_sum_t& delta_stats);
+  virtual rep_op_fut_t
+  submit_transaction(const std::set<pg_shard_t> &pg_shards,
+		     const hobject_t& hoid,
+		     ceph::os::Transaction&& txn,
+		     osd_op_params_t&& osd_op_p,
+		     epoch_t min_epoch, epoch_t max_epoch,
+		     std::vector<pg_log_entry_t>&& log_entries) = 0;
 
   virtual void got_rep_op_reply(const MOSDRepOpReply&) {}
   virtual seastar::future<> stop() = 0;
@@ -475,13 +474,6 @@ class PGBackend
     object_stat_sum_t& delta_stats,
     object_info_t& oi,
     uint64_t truncate_size);
-  virtual rep_op_fut_t
-  _submit_transaction(std::set<pg_shard_t>&& pg_shards,
-		      const hobject_t& hoid,
-		      ceph::os::Transaction&& txn,
-		      osd_op_params_t&& osd_op_p,
-		      epoch_t min_epoch, epoch_t max_epoch,
-		      std::vector<pg_log_entry_t>&& log_entries) = 0;
   friend class ReplicatedRecoveryBackend;
   friend class ::crimson::osd::PG;
 

src/crimson/osd/replicated_backend.cc

@@ -36,14 +36,14 @@ ReplicatedBackend::_read(const hobject_t& hoid,
 }
 
 ReplicatedBackend::rep_op_fut_t
-ReplicatedBackend::_submit_transaction(std::set<pg_shard_t>&& pg_shards,
-                                       const hobject_t& hoid,
-                                       ceph::os::Transaction&& txn,
-                                       osd_op_params_t&& osd_op_p,
-                                       epoch_t min_epoch, epoch_t map_epoch,
-				       std::vector<pg_log_entry_t>&& log_entries)
+ReplicatedBackend::submit_transaction(const std::set<pg_shard_t>& pg_shards,
+                                      const hobject_t& hoid,
+                                      ceph::os::Transaction&& txn,
+                                      osd_op_params_t&& osd_op_p,
+                                      epoch_t min_epoch, epoch_t map_epoch,
+				      std::vector<pg_log_entry_t>&& log_entries)
 {
-  LOG_PREFIX(ReplicatedBackend::_submit_transaction);
+  LOG_PREFIX(ReplicatedBackend::submit_transaction);
   DEBUGDPP("object {}", dpp, hoid);
 
   const ceph_tid_t tid = shard_services.get_tid();

src/crimson/osd/replicated_backend.h

@@ -32,7 +32,8 @@ class ReplicatedBackend : public PGBackend
   ll_read_ierrorator::future<ceph::bufferlist>
     _read(const hobject_t& hoid, uint64_t off,
 	  uint64_t len, uint32_t flags) override;
-  rep_op_fut_t _submit_transaction(std::set<pg_shard_t>&& pg_shards,
+  rep_op_fut_t submit_transaction(
+    const std::set<pg_shard_t> &pg_shards,
     const hobject_t& hoid,
     ceph::os::Transaction&& txn,
     osd_op_params_t&& osd_op_p,