From b9ef4367229f3afc2a930e65aacf1d2eea741fb2 Mon Sep 17 00:00:00 2001 From: Xuehan Xu Date: Wed, 11 Sep 2024 15:55:16 +0800 Subject: [PATCH] crimson/osd/ops_executor: simplify OpsExecuter::rollback_obc_if_modified Signed-off-by: Xuehan Xu --- src/crimson/osd/ops_executer.h | 12 +----- src/crimson/osd/pg.cc | 75 +++++++++++++++------------------- 2 files changed, 36 insertions(+), 51 deletions(-) diff --git a/src/crimson/osd/ops_executer.h b/src/crimson/osd/ops_executer.h index 623e3d90d56..c06f6dd445a 100644 --- a/src/crimson/osd/ops_executer.h +++ b/src/crimson/osd/ops_executer.h @@ -571,7 +571,7 @@ OpsExecuter::flush_changes_n_do_ops_effects( template 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 -OpsExecuter::interruptible_future<> -OpsExecuter::RollbackHelper::rollback_obc_if_modified( +void OpsExecuter::RollbackHelper::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::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::rollback_obc_if_modified( if (need_rollback) { func(ox->obc); } - return interruptor::now(); } // PgOpsExecuter -- a class for executing ops targeting a certain PG. diff --git a/src/crimson/osd/pg.cc b/src/crimson/osd/pg.cc index 6038dd655a4..50f994805da 100644 --- a/src/crimson/osd/pg.cc +++ b/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{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{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>( @@ -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::nullopt); - // call submit_error_log only for non-internal clients - if constexpr (!std::is_same_v) { - 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::nullopt); + // call submit_error_log only for non-internal clients + if constexpr (!std::is_same_v) { + 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>( - std::move(seastar::now()), - std::move(all_completed()) - ); - }); + }); + } else { + return (*failure_func_ptr)(e); + } + }; + return PG::do_osd_ops_iertr::make_ready_future>( + std::move(seastar::now()), + std::move(all_completed()) + ); }); })); } -- 2.39.5