crimson/osd: delegate the txn.empty() dispatch to OpsExecuter.

The benefit is no need to instantiate `osd_op_params` when only
non-modifying operation had been handled.

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

diff --git a/src/crimson/osd/ops_executer.h b/src/crimson/osd/ops_executer.h
index 036c02632c02368d54f6fb54c89bbbcd72216386..0a224418fb916e710968fb6d5ddb0d0421c940d1 100644 (file)
--- a/src/crimson/osd/ops_executer.h
+++ b/src/crimson/osd/ops_executer.h
@@ -182,8 +182,8 @@ public:
   osd_op_errorator::future<> execute_osd_op(class OSDOp& osd_op);
   seastar::future<> execute_pg_op(class OSDOp& osd_op);
 
-  template <typename Func>
-  osd_op_errorator::future<> submit_changes(Func&& f) &&;
+  template <typename Func, typename MutFunc>
+  osd_op_errorator::future<> flush_changes(Func&& func, MutFunc&& mut_func) &&;
 
   const auto& get_message() const {
     return *msg;
@@ -207,7 +207,7 @@ auto OpsExecuter::with_effect_on_obc(
   using context_t = std::decay_t<Context>;
   // the language offers implicit conversion to pointer-to-function for
   // lambda only when it's closureless. We enforce this restriction due
-  // the fact that `submit_changes()` std::moves many executer's parts.
+  // the fact that `flush_changes()` std::moves many executer's parts.
   using allowed_effect_func_t =
     seastar::future<> (*)(context_t&&, ObjectContextRef);
   static_assert(std::is_convertible_v<EffectFunc, allowed_effect_func_t>,
@@ -233,30 +233,44 @@ auto OpsExecuter::with_effect_on_obc(
   return std::forward<MainFunc>(main_func)(ctx_ref);
 }
 
-template <typename Func>
-OpsExecuter::osd_op_errorator::future<> OpsExecuter::submit_changes(Func&& f) && {
+template <typename Func,
+          typename MutFunc>
+OpsExecuter::osd_op_errorator::future<> OpsExecuter::flush_changes(
+  Func&& func,
+  MutFunc&& mut_func) &&
+{
   assert(obc);
-  if (!osd_op_params) {
-    osd_op_params = osd_op_params_t();
+  const bool want_mutate = !txn.empty();
+  if (want_mutate) {
+    if (!osd_op_params) {
+      osd_op_params = osd_op_params_t();
+    }
+    osd_op_params->req = std::move(msg);
+    osd_op_params->at_version = pg.next_version();
+    osd_op_params->pg_trim_to = pg.get_pg_trim_to();
+    osd_op_params->min_last_complete_ondisk = pg.get_min_last_complete_ondisk();
+    osd_op_params->last_complete = pg.get_info().last_complete;
+    if (user_modify) {
+      osd_op_params->user_at_version = osd_op_params->at_version.version;
+    }
   }
-  osd_op_params->req = std::move(msg);
-  eversion_t at_version = pg.next_version();
-
-  osd_op_params->at_version = at_version;
-  osd_op_params->pg_trim_to = pg.get_pg_trim_to();
-  osd_op_params->min_last_complete_ondisk = pg.get_min_last_complete_ondisk();
-  osd_op_params->last_complete = pg.get_info().last_complete;
-  if (user_modify)
-    osd_op_params->user_at_version = at_version.version;
   if (__builtin_expect(op_effects.empty(), true)) {
-    return std::forward<Func>(f)(std::move(txn), std::move(obc), std::move(*osd_op_params));
-  }
-  return std::forward<Func>(f)(std::move(txn), std::move(obc), std::move(*osd_op_params)).safe_then([this] {
-    // let's do the cleaning of `op_effects` in destructor
-    return crimson::do_for_each(op_effects, [] (auto& op_effect) {
-      return op_effect->execute();
+    return want_mutate ? std::forward<MutFunc>(mut_func)(std::move(txn),
+                                                         std::move(obc),
+                                                         std::move(*osd_op_params))
+                       : std::forward<Func>(func)(std::move(obc));
+  } else {
+    return (want_mutate ? std::forward<MutFunc>(mut_func)(std::move(txn),
+                                                          std::move(obc),
+                                                          std::move(*osd_op_params))
+                        : std::forward<Func>(func)(std::move(obc))
+    ).safe_then([this] {
+      // let's do the cleaning of `op_effects` in destructor
+      return crimson::do_for_each(op_effects, [] (auto& op_effect) {
+        return op_effect->execute();
+      });
     });
-  });
+  }
 }
 
 } // namespace crimson::osd

diff --git a/src/crimson/osd/pg.cc b/src/crimson/osd/pg.cc
index 44d3cd2249d8e25fb922b754df064792490acc1c..5b2b64fb3fd1f83017b97a4f2efacce172dc240f 100644 (file)
--- a/src/crimson/osd/pg.cc
+++ b/src/crimson/osd/pg.cc
@@ -606,26 +606,23 @@ seastar::future<Ref<MOSDOpReply>> PG::do_osd_ops(
       "do_osd_ops: {} - object {} all operations successful",
       *m,
       obc->obs.oi.soid);
-    return std::move(*ox).submit_changes([this, m, &op_info]
-      (auto&& txn, auto&& obc, auto&& osd_op_p) -> osd_op_errorator::future<> {
-       // XXX: the entire lambda could be scheduled conditionally. ::if_then()?
-       if (txn.empty()) {
-         logger().debug(
-           "do_osd_ops: {} - object {} txn is empty, bypassing mutate",
-           *m,
-           obc->obs.oi.soid);
-          return osd_op_errorator::now();
-        } else {
-         logger().debug(
-           "do_osd_ops: {} - object {} submitting txn",
-           *m,
-           obc->obs.oi.soid);
-          return submit_transaction(op_info,
-                                     m->ops,
-                                     std::move(obc),
-                                     std::move(txn),
-                                     std::move(osd_op_p));
-        }
+    return std::move(*ox).flush_changes(
+      [this, m] (auto&& obc) -> osd_op_errorator::future<> {
+       logger().debug(
+         "do_osd_ops: {} - object {} txn is empty, bypassing mutate",
+         *m,
+         obc->obs.oi.soid);
+        return osd_op_errorator::now();
+      },
+      [this, m, &op_info] (auto&& txn,
+                          auto&& obc,
+                          auto&& osd_op_p) -> osd_op_errorator::future<> {
+       logger().debug(
+         "do_osd_ops: {} - object {} submitting txn",
+         *m,
+         obc->obs.oi.soid);
+       return submit_transaction(
+          op_info, m->ops, std::move(obc), std::move(txn), std::move(osd_op_p));
       });
   }).safe_then([this,
                 m,