From 13ef6dcead0f4e4f49de27891c4f95702696665f Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Tue, 4 Aug 2020 15:59:55 +0200 Subject: [PATCH] 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 --- src/crimson/osd/ops_executer.h | 60 +++++++++++++++++++++------------- src/crimson/osd/pg.cc | 37 ++++++++++----------- 2 files changed, 54 insertions(+), 43 deletions(-) diff --git a/src/crimson/osd/ops_executer.h b/src/crimson/osd/ops_executer.h index 036c02632c0..0a224418fb9 100644 --- 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 - osd_op_errorator::future<> submit_changes(Func&& f) &&; + template + 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; // 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, @@ -233,30 +233,44 @@ auto OpsExecuter::with_effect_on_obc( return std::forward(main_func)(ctx_ref); } -template -OpsExecuter::osd_op_errorator::future<> OpsExecuter::submit_changes(Func&& f) && { +template +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(f)(std::move(txn), std::move(obc), std::move(*osd_op_params)); - } - return std::forward(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(mut_func)(std::move(txn), + std::move(obc), + std::move(*osd_op_params)) + : std::forward(func)(std::move(obc)); + } else { + return (want_mutate ? std::forward(mut_func)(std::move(txn), + std::move(obc), + std::move(*osd_op_params)) + : std::forward(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 44d3cd2249d..5b2b64fb3fd 100644 --- a/src/crimson/osd/pg.cc +++ b/src/crimson/osd/pg.cc @@ -606,26 +606,23 @@ seastar::future> 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, -- 2.39.5