From 0136695af5b8cc5892f9a4630d7c0093c629c7f3 Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Fri, 20 Nov 2020 14:31:47 +0100 Subject: [PATCH] crimson: make OpsExecuter unaware about PG. `PG` is heavy-weight class with many responsibilities. Exposing it to lower-layer may suggest there is far more coupling between them than in reality. Signed-off-by: Radoslaw Zarzynski --- src/crimson/osd/ops_executer.cc | 1 + src/crimson/osd/ops_executer.h | 38 +++++++++++++++------------------ src/crimson/osd/pg.cc | 36 +++++++++++++++++++++++++------ src/crimson/osd/pg.h | 4 ++++ 4 files changed, 51 insertions(+), 28 deletions(-) diff --git a/src/crimson/osd/ops_executer.cc b/src/crimson/osd/ops_executer.cc index 027fac9a7039e..6b6614e93d884 100644 --- a/src/crimson/osd/ops_executer.cc +++ b/src/crimson/osd/ops_executer.cc @@ -16,6 +16,7 @@ #include #include "crimson/osd/exceptions.h" +#include "crimson/osd/pg.h" #include "crimson/osd/watch.h" #include "osd/ClassHandler.h" diff --git a/src/crimson/osd/ops_executer.h b/src/crimson/osd/ops_executer.h index 0efe6b43c7a75..b05a99911594d 100644 --- a/src/crimson/osd/ops_executer.h +++ b/src/crimson/osd/ops_executer.h @@ -25,12 +25,12 @@ #include "crimson/osd/shard_services.h" #include "crimson/osd/osdmap_gate.h" -#include "crimson/osd/pg.h" #include "crimson/osd/pg_backend.h" #include "crimson/osd/exceptions.h" #include "messages/MOSDOp.h" +class PG; class PGLSFilter; class OSDOp; @@ -85,7 +85,7 @@ private: ObjectContextRef obc; const OpInfo& op_info; - PG& pg; + const pg_pool_t& pool_info; // for the sake of the ObjClass API PGBackend& backend; Ref msg; std::optional osd_op_params; @@ -166,11 +166,15 @@ private: } public: - OpsExecuter(ObjectContextRef obc, const OpInfo& op_info, PG& pg, Ref msg) + OpsExecuter(ObjectContextRef obc, + const OpInfo& op_info, + const pg_pool_t& pool_info, + PGBackend& backend, + Ref msg) : obc(std::move(obc)), op_info(op_info), - pg(pg), - backend(pg.get_backend()), + pool_info(pool_info), + backend(backend), msg(std::move(msg)) { } @@ -188,7 +192,7 @@ public: } uint32_t get_pool_stripe_width() const { - return pg.get_pool().info.get_stripe_width(); + return pool_info.get_stripe_width(); } }; @@ -233,29 +237,21 @@ OpsExecuter::osd_op_errorator::future<> OpsExecuter::flush_changes( Func&& func, MutFunc&& mut_func) && { - assert(obc); const bool want_mutate = !txn.empty(); - if (want_mutate) { - // osd_op_params are instantiated by every wr-like operation. - assert(osd_op_params); - 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 are instantiated by every wr-like operation. + assert(osd_op_params || !want_mutate); + assert(obc); if (__builtin_expect(op_effects.empty(), true)) { return want_mutate ? std::forward(mut_func)(std::move(txn), std::move(obc), - std::move(*osd_op_params)) + std::move(*osd_op_params), + user_modify) : 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::move(*osd_op_params), + user_modify) : std::forward(func)(std::move(obc)) ).safe_then([this] { // let's do the cleaning of `op_effects` in destructor diff --git a/src/crimson/osd/pg.cc b/src/crimson/osd/pg.cc index 38c913c4d2396..2b17b37d3c423 100644 --- a/src/crimson/osd/pg.cc +++ b/src/crimson/osd/pg.cc @@ -596,6 +596,22 @@ seastar::future<> PG::submit_transaction(const OpInfo& op_info, }); } +osd_op_params_t&& PG::fill_op_params_bump_pg_version( + osd_op_params_t&& osd_op_p, + Ref m, + const bool user_modify) +{ + osd_op_p.req = std::move(m); + osd_op_p.at_version = next_version(); + osd_op_p.pg_trim_to = get_pg_trim_to(); + osd_op_p.min_last_complete_ondisk = get_min_last_complete_ondisk(); + osd_op_p.last_complete = get_info().last_complete; + if (user_modify) { + osd_op_p.user_at_version = osd_op_p.at_version.version; + } + return std::move(osd_op_p); +} + seastar::future> PG::do_osd_ops( Ref m, ObjectContextRef obc, @@ -608,11 +624,8 @@ seastar::future> PG::do_osd_ops( using osd_op_errorator = OpsExecuter::osd_op_errorator; const auto oid = m->get_snapid() == CEPH_SNAPDIR ? m->get_hobj().get_head() : m->get_hobj(); - // OpsExecuter gets PG as non-const as it's responsible for modying - // e.g. `projected_last_update`. - auto ox = - std::make_unique(obc, op_info, *this, m); - + auto ox = std::make_unique( + obc, op_info, get_pool().info, get_backend(), m); return crimson::do_for_each( m->ops, [obc, m, ox = ox.get()](OSDOp& osd_op) { logger().debug( @@ -636,13 +649,22 @@ seastar::future> PG::do_osd_ops( }, [this, m, &op_info] (auto&& txn, auto&& obc, - auto&& osd_op_p) -> osd_op_errorator::future<> { + auto&& osd_op_p, + bool user_modify) -> osd_op_errorator::future<> { logger().debug( "do_osd_ops: {} - object {} submitting txn", *m, obc->obs.oi.soid); + auto filled_osd_op_p = fill_op_params_bump_pg_version( + std::move(osd_op_p), + std::move(m), + user_modify); return submit_transaction( - op_info, m->ops, std::move(obc), std::move(txn), std::move(osd_op_p)); + op_info, + filled_osd_op_p.req->ops, + std::move(obc), + std::move(txn), + std::move(filled_osd_op_p)); }); }).safe_then([this, m, diff --git a/src/crimson/osd/pg.h b/src/crimson/osd/pg.h index 33782da01d3ef..4a2715fbc9a17 100644 --- a/src/crimson/osd/pg.h +++ b/src/crimson/osd/pg.h @@ -531,6 +531,10 @@ private: void do_peering_event( const boost::statechart::event_base &evt, PeeringCtx &rctx); + osd_op_params_t&& fill_op_params_bump_pg_version( + osd_op_params_t&& osd_op_p, + Ref m, + const bool user_modify); seastar::future> do_osd_ops( Ref m, ObjectContextRef obc, -- 2.39.5