crimson/osd/ops_executor: simplify OpsExecuter::rollback_obc_if_modified

Signed-off-by: Xuehan Xu <[email protected]>

src/crimson/osd/ops_executer.h

@@ -571,7 +571,7 @@ OpsExecuter::flush_changes_n_do_ops_effects(
 
 template <class Func>
 struct OpsExecuter::RollbackHelper {
-  interruptible_future<> rollback_obc_if_modified(const std::error_code& e);
+  void rollback_obc_if_modified(const std::error_code& e);
   ObjectContextRef get_obc() const {
     assert(ox);
     return ox->obc;
@@ -588,8 +588,7 @@ OpsExecuter::create_rollbacker(Func&& func) {
 
 
 template <class Func>
-OpsExecuter::interruptible_future<>
-OpsExecuter::RollbackHelper<Func>::rollback_obc_if_modified(
+void OpsExecuter::RollbackHelper<Func>::rollback_obc_if_modified(
   const std::error_code& e)
 {
   // Oops, an operation had failed. do_osd_ops() altogether with
@@ -599,12 +598,6 @@ OpsExecuter::RollbackHelper<Func>::rollback_obc_if_modified(
   // we maintain and we did it for both reading and writing.
   // Now all modifications must be reverted.
   //
-  // Let's just reload from the store. Evicting from the shared
-  // LRU would be tricky as next MOSDOp (the one at `get_obc`
-  // phase) could actually already finished the lookup. Fortunately,
-  // this is supposed to live on cold  paths, so performance is not
-  // a concern -- simplicity wins.
-  //
   // The conditional's purpose is to efficiently handle hot errors
   // which may appear as a result of e.g. CEPH_OSD_OP_CMPXATTR or
   // CEPH_OSD_OP_OMAP_CMP. These are read-like ops and clients
@@ -622,7 +615,6 @@ OpsExecuter::RollbackHelper<Func>::rollback_obc_if_modified(
   if (need_rollback) {
     func(ox->obc);
   }
-  return interruptor::now();
 }
 
 // PgOpsExecuter -- a class for executing ops targeting a certain PG.

src/crimson/osd/pg.cc

@@ -1058,13 +1058,12 @@ PG::do_osd_ops_execute(
       // this is a path for EIO. it's special because we want to fix the obejct
       // and try again. that is, the layer above `PG::do_osd_ops` is supposed to
       // restart the execution.
-      return rollbacker.rollback_obc_if_modified(e).then_interruptible(
-      [obc=rollbacker.get_obc(), this] {
-        return repair_object(obc->obs.oi.soid,
-                             obc->obs.oi.version
-        ).then_interruptible([] {
-          return do_osd_ops_iertr::future<Ret>{crimson::ct_error::eagain::make()};
-        });
+      rollbacker.rollback_obc_if_modified(e);
+      auto obc = rollbacker.get_obc();
+      return repair_object(obc->obs.oi.soid,
+                           obc->obs.oi.version
+      ).then_interruptible([] {
+        return do_osd_ops_iertr::future<Ret>{crimson::ct_error::eagain::make()};
       });
     }), OpsExecuter::osd_op_errorator::all_same_way(
         [rollbacker, failure_func_ptr]
@@ -1073,11 +1072,8 @@ PG::do_osd_ops_execute(
           ceph_assert(e.value() == EDQUOT ||
                       e.value() == ENOSPC ||
                       e.value() == EAGAIN);
-          return rollbacker.rollback_obc_if_modified(e).then_interruptible(
-          [e, failure_func_ptr] {
-            // no need to record error log
-            return (*failure_func_ptr)(e);
-          });
+          rollbacker.rollback_obc_if_modified(e);
+          return (*failure_func_ptr)(e);
     }));
 
     return PG::do_osd_ops_iertr::make_ready_future<pg_rep_op_fut_t<Ret>>(
@@ -1089,37 +1085,34 @@ PG::do_osd_ops_execute(
      rollbacker, failure_func_ptr]
     (const std::error_code& e) mutable {
     ceph_tid_t rep_tid = shard_services.get_tid();
-    return rollbacker.rollback_obc_if_modified(e).then_interruptible(
-    [&, op_info, m, obc,
-     this, e, rep_tid, failure_func_ptr] {
-      // record error log
-      auto maybe_submit_error_log =
-        seastar::make_ready_future<std::optional<eversion_t>>(std::nullopt);
-      // call submit_error_log only for non-internal clients
-      if constexpr (!std::is_same_v<Ret, void>) {
-        if(op_info.may_write()) {
-          maybe_submit_error_log =
-            submit_error_log(m, op_info, obc, e, rep_tid);
-        }
+    rollbacker.rollback_obc_if_modified(e);
+    // record error log
+    auto maybe_submit_error_log =
+      seastar::make_ready_future<std::optional<eversion_t>>(std::nullopt);
+    // call submit_error_log only for non-internal clients
+    if constexpr (!std::is_same_v<Ret, void>) {
+      if(op_info.may_write()) {
+        maybe_submit_error_log =
+          submit_error_log(m, op_info, obc, e, rep_tid);
       }
-      return maybe_submit_error_log.then(
-      [this, failure_func_ptr, e, rep_tid] (auto version) {
-        auto all_completed =
-        [this, failure_func_ptr, e, rep_tid,  version] {
-          if (version.has_value()) {
-            return complete_error_log(rep_tid, version.value()).then(
-            [failure_func_ptr, e] {
-              return (*failure_func_ptr)(e);
-            });
-          } else {
+    }
+    return maybe_submit_error_log.then(
+    [this, failure_func_ptr, e, rep_tid] (auto version) {
+      auto all_completed =
+      [this, failure_func_ptr, e, rep_tid,  version] {
+        if (version.has_value()) {
+          return complete_error_log(rep_tid, version.value()).then(
+          [failure_func_ptr, e] {
             return (*failure_func_ptr)(e);
-          }
-        };
-        return PG::do_osd_ops_iertr::make_ready_future<pg_rep_op_fut_t<Ret>>(
-          std::move(seastar::now()),
-          std::move(all_completed())
-        );
-      });
+          });
+        } else {
+          return (*failure_func_ptr)(e);
+        }
+      };
+      return PG::do_osd_ops_iertr::make_ready_future<pg_rep_op_fut_t<Ret>>(
+        std::move(seastar::now()),
+        std::move(all_completed())
+      );
     });
   }));
 }