From 17c9827ad8882c7945964c65734dfe329851e562 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 26 Nov 2020 18:39:15 +0800 Subject: [PATCH] crimson/osd: errororize PG::with_*_obc methods() they all use `seastar::with_lock()` to ensure the obc lock is released in the error handling path. but we cannot assume that `seastar::with_lock()` always return `seastar::future<>`, so propogate the error returned by the function passed to `with_lock()` using errorator. Signed-off-by: Kefu Chai --- src/crimson/osd/pg.cc | 21 ++++++++++--------- src/crimson/osd/pg.h | 7 ++++--- .../osd/replicated_recovery_backend.cc | 17 ++++++++------- src/crimson/osd/replicated_recovery_backend.h | 4 +++- 4 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/crimson/osd/pg.cc b/src/crimson/osd/pg.cc index 7598326379c..5f8129c62ca 100644 --- a/src/crimson/osd/pg.cc +++ b/src/crimson/osd/pg.cc @@ -795,7 +795,7 @@ std::optional PG::resolve_oid( } template -seastar::future<> +PG::load_obc_ertr::future<> PG::with_head_obc(hobject_t oid, with_obc_func_t&& func) { assert(oid.is_head()); @@ -803,7 +803,7 @@ PG::with_head_obc(hobject_t oid, with_obc_func_t&& func) return obc->with_lock( [oid=std::move(oid), existed=existed, obc=std::move(obc), func=std::move(func), this] { - auto loaded = seastar::make_ready_future(obc); + auto loaded = load_obc_ertr::make_ready_future(obc); if (existed) { logger().debug("with_head_obc: found {} in cache", oid); } else { @@ -812,30 +812,31 @@ PG::with_head_obc(hobject_t oid, with_obc_func_t&& func) return load_head_obc(obc); }); } - return loaded.then([func = std::move(func)](auto obc) { + return loaded.safe_then([func=std::move(func)](auto obc) { return func(std::move(obc)); }); }); } template -seastar::future<> +PG::load_obc_ertr::future<> PG::with_clone_obc(hobject_t oid, with_obc_func_t&& func) { assert(!oid.is_head()); return with_head_obc(oid.get_head(), - [oid, func=std::move(func), this](auto head) { + [oid, func=std::move(func), this](auto head) -> load_obc_ertr::future<> { auto coid = resolve_oid(head->get_ro_ss(), oid); if (!coid) { // TODO: return crimson::ct_error::enoent::make(); logger().error("with_clone_obc: {} clone not found", coid); - return seastar::make_ready_future<>(); + return load_obc_ertr::make_ready_future<>(); } auto [clone, existed] = shard_services.obc_registry.get_cached_obc(*coid); return clone->template with_lock( [coid=*coid, existed=existed, - head=std::move(head), clone=std::move(clone), func=std::move(func), this] { - auto loaded = seastar::make_ready_future(clone); + head=std::move(head), clone=std::move(clone), + func=std::move(func), this]() -> load_obc_ertr::future<> { + auto loaded = load_obc_ertr::make_ready_future(clone); if (existed) { logger().debug("with_clone_obc: found {} in cache", coid); } else { @@ -849,7 +850,7 @@ PG::with_clone_obc(hobject_t oid, with_obc_func_t&& func) }); }); } - return loaded.then([func = std::move(func)](auto clone) { + return loaded.safe_then([func=std::move(func)](auto clone) { return func(std::move(clone)); }); }); @@ -857,7 +858,7 @@ PG::with_clone_obc(hobject_t oid, with_obc_func_t&& func) } // explicitly instantiate the used instantiations -template seastar::future<> +template PG::load_obc_ertr::future<> PG::with_head_obc(hobject_t, with_obc_func_t&&); PG::load_obc_ertr::future diff --git a/src/crimson/osd/pg.h b/src/crimson/osd/pg.h index bccb74b198a..d81d81f8f79 100644 --- a/src/crimson/osd/pg.h +++ b/src/crimson/osd/pg.h @@ -501,10 +501,11 @@ public: load_head_obc(ObjectContextRef obc); public: - using with_obc_func_t = std::function (ObjectContextRef)>; + using with_obc_func_t = + std::function (ObjectContextRef)>; template - seastar::future<> with_head_obc(hobject_t oid, with_obc_func_t&& func); + load_obc_ertr::future<> with_head_obc(hobject_t oid, with_obc_func_t&& func); load_obc_ertr::future<> with_locked_obc( Ref &m, @@ -521,7 +522,7 @@ public: private: template - seastar::future<> with_clone_obc(hobject_t oid, with_obc_func_t&& func); + load_obc_ertr::future<> with_clone_obc(hobject_t oid, with_obc_func_t&& func); load_obc_ertr::future get_locked_obc( Operation *op, diff --git a/src/crimson/osd/replicated_recovery_backend.cc b/src/crimson/osd/replicated_recovery_backend.cc index edd2513e97a..42fc442ec43 100644 --- a/src/crimson/osd/replicated_recovery_backend.cc +++ b/src/crimson/osd/replicated_recovery_backend.cc @@ -30,20 +30,20 @@ seastar::future<> ReplicatedRecoveryBackend::recover_object( [this, soid, need](auto& pops, auto& shards) { return maybe_pull_missing_obj(soid, need).then([this, soid](bool pulled) { return load_obc_for_recovery(soid, pulled); - }).then([this, soid, need, &pops, &shards] { + }).safe_then([this, soid, need, &pops, &shards] { if (!shards.empty()) { return prep_push(soid, need, &pops, shards); } else { return seastar::now(); } - }).handle_exception([this, soid](auto e) { + }, crimson::ct_error::all_same_way([this, soid](const std::error_code& e) { auto recovery_waiter = recovering.find(soid); if (auto obc = recovery_waiter->second.obc; obc) { obc->drop_recovery_read(); } recovering.erase(recovery_waiter); return seastar::make_exception_future<>(e); - }).then([this, &pops, &shards, soid] { + })).then([this, &pops, &shards, soid] { return seastar::parallel_for_each(shards, [this, &pops, soid](auto shard) { auto msg = make_message(); @@ -114,13 +114,14 @@ ReplicatedRecoveryBackend::maybe_pull_missing_obj( }); } -seastar::future<> ReplicatedRecoveryBackend::load_obc_for_recovery( +auto ReplicatedRecoveryBackend::load_obc_for_recovery( const hobject_t& soid, - bool pulled) + bool pulled) -> + load_obc_ertr::future<> { auto& recovery_waiter = recovering.at(soid); if (recovery_waiter.obc) { - return seastar::now(); + return load_obc_ertr::now(); } return pg.with_head_obc(soid, [&recovery_waiter](auto obc) { logger().debug("load_obc_for_recovery: loaded obc: {}", obc->obs.oi.soid); @@ -649,8 +650,8 @@ seastar::future ReplicatedRecoveryBackend::_handle_pull_response( recovery_waiter.obc = obc; obc->obs.oi.decode(pop.attrset[OI_ATTR]); pi.recovery_info.oi = obc->obs.oi; - return seastar::make_ready_future<>(); - }); + return crimson::osd::PG::load_obc_ertr::now(); + }).handle_error(crimson::ct_error::assert_all{}); }; return prepare_waiter.then([this, first=pi.recovery_progress.first, &pi, &pop, t, response]() mutable { diff --git a/src/crimson/osd/replicated_recovery_backend.h b/src/crimson/osd/replicated_recovery_backend.h index 4bb353c5aa0..919aa3a703b 100644 --- a/src/crimson/osd/replicated_recovery_backend.h +++ b/src/crimson/osd/replicated_recovery_backend.h @@ -117,7 +117,9 @@ private: eversion_t need); /// load object context for recovery if it is not ready yet - seastar::future<> load_obc_for_recovery( + using load_obc_ertr = crimson::errorator< + crimson::ct_error::object_corrupted>; + load_obc_ertr::future<> load_obc_for_recovery( const hobject_t& soid, bool pulled); -- 2.39.5