From ae28c1489543bff5c97c8acbf56ff3282b824528 Mon Sep 17 00:00:00 2001 From: Alex Ainscow Date: Thu, 27 Mar 2025 14:38:44 +0000 Subject: [PATCH] crimson/osd: Add scrub stubs for crimson and classic, ready for new EC The new optimised EC code is not backward compatible withold EC Code. Before this commit there is some stub code which assumes that an hinfo xattr will exist and can be used for scrub. This is no longer the case in new EC. We plan to first make the scrubbing changes for new EC in classic and will subsequently port to crimson. It will not look like the code here, so there is little point in keeping it. Additionally, add some stubs for scrub in classic optimized EC. There will be a later PR specifically for dealing with scrubbing in new EC which fix all the fix mes in class, The crimson code will be fixed up at a later date and will only support optimised EC. Signed-off-by: Alex Ainscow --- src/crimson/osd/scrub/pg_scrubber.cc | 1 - src/crimson/osd/scrub/scrub_validator.h | 8 +- src/osd/ECBackend.h | 2 +- src/osd/ECSwitch.h | 9 +- src/osd/PG.h | 6 +- src/osd/PGBackend.h | 4 +- src/osd/ReplicatedBackend.h | 3 +- src/osd/scrubber/scrub_backend.cc | 20 ++-- src/osd/scrubber/scrub_backend.h | 3 +- src/osd/scrubber_common.h | 3 +- src/test/crimson/test_crimson_scrub.cc | 153 ++---------------------- src/test/osd/test_scrubber_be.cc | 3 +- 12 files changed, 41 insertions(+), 174 deletions(-) diff --git a/src/crimson/osd/scrub/pg_scrubber.cc b/src/crimson/osd/scrub/pg_scrubber.cc index 3ac2c910db2..0ad206ffe75 100644 --- a/src/crimson/osd/scrub/pg_scrubber.cc +++ b/src/crimson/osd/scrub/pg_scrubber.cc @@ -145,7 +145,6 @@ chunk_validation_policy_t PGScrubber::get_policy() const { return chunk_validation_policy_t{ pg.get_primary(), - std::nullopt /* stripe_info, populate when EC is implemented */, crimson::common::local_conf().get_val( "osd_max_object_size"), crimson::common::local_conf().get_val( diff --git a/src/crimson/osd/scrub/scrub_validator.h b/src/crimson/osd/scrub/scrub_validator.h index 32f5933d0db..2bbcd023c34 100644 --- a/src/crimson/osd/scrub/scrub_validator.h +++ b/src/crimson/osd/scrub/scrub_validator.h @@ -9,14 +9,12 @@ #include "common/config_proxy.h" #include "common/scrub_types.h" #include "crimson/common/log.h" -#include "osd/ECUtil.h" #include "osd/osd_types.h" namespace crimson::osd::scrub { struct chunk_validation_policy_t { pg_shard_t primary; - std::optional stripe_info; // osd_max_object_size size_t max_object_size; @@ -31,11 +29,13 @@ struct chunk_validation_policy_t { bool is_ec() const { - return !!stripe_info; + // FIXME: See scrub_backend in classic for reference. + return false; } size_t logical_to_ondisk_size(size_t size) const { - return stripe_info ? stripe_info->logical_to_next_chunk_offset(size) : size; + // FIXME: See scrub_backend in classic for how to handle EC. + return size; } }; diff --git a/src/osd/ECBackend.h b/src/osd/ECBackend.h index 8b50cc22e48..715eb97c743 100644 --- a/src/osd/ECBackend.h +++ b/src/osd/ECBackend.h @@ -447,7 +447,7 @@ public: ScrubMapBuilder &pos, ScrubMap::object &o); - uint64_t be_get_ondisk_size(uint64_t logical_size) const { + uint64_t be_get_ondisk_size(uint64_t logical_size, shard_id_t ignored) const { return sinfo.logical_to_next_chunk_offset(logical_size); } }; diff --git a/src/osd/ECSwitch.h b/src/osd/ECSwitch.h index a5e81e8cbb9..4069662ade0 100644 --- a/src/osd/ECSwitch.h +++ b/src/osd/ECSwitch.h @@ -291,10 +291,11 @@ public: return legacy.auto_repair_supported(); } - uint64_t be_get_ondisk_size(uint64_t logical_size) const final - { - if (is_optimized()) { - return optimized.be_get_ondisk_size(logical_size); + uint64_t be_get_ondisk_size(uint64_t logical_size, + shard_id_t shard_id) const final { + if (is_optimized()) + { + return optimized.be_get_ondisk_size(logical_size, shard_id); } return legacy.be_get_ondisk_size(logical_size); } diff --git a/src/osd/PG.h b/src/osd/PG.h index 7ab0ece26f8..7ac621cdffe 100644 --- a/src/osd/PG.h +++ b/src/osd/PG.h @@ -1395,9 +1395,9 @@ public: recovery_state.force_object_missing(peer, oid, version); } - uint64_t logical_to_ondisk_size(uint64_t logical_size) const final - { - return get_pgbackend()->be_get_ondisk_size(logical_size); + uint64_t logical_to_ondisk_size(uint64_t logical_size, + int8_t shard_id) const final { + return get_pgbackend()->be_get_ondisk_size(logical_size, shard_id_t(shard_id)); } }; diff --git a/src/osd/PGBackend.h b/src/osd/PGBackend.h index 4e5be28b637..c809469ae50 100644 --- a/src/osd/PGBackend.h +++ b/src/osd/PGBackend.h @@ -602,8 +602,8 @@ typedef std::shared_ptr OSDMapRef; ScrubMap &map, ScrubMapBuilder &pos); - virtual uint64_t be_get_ondisk_size( - uint64_t logical_size) const = 0; + virtual uint64_t be_get_ondisk_size(uint64_t logical_size, + shard_id_t shard_id) const = 0; virtual int be_deep_scrub( const hobject_t &oid, diff --git a/src/osd/ReplicatedBackend.h b/src/osd/ReplicatedBackend.h index 7415a40c325..5c22d19a121 100644 --- a/src/osd/ReplicatedBackend.h +++ b/src/osd/ReplicatedBackend.h @@ -462,7 +462,8 @@ private: ScrubMapBuilder &pos, ScrubMap::object &o) override; - uint64_t be_get_ondisk_size(uint64_t logical_size) const final { + uint64_t be_get_ondisk_size(uint64_t logical_size, + shard_id_t unused) const final { return logical_size; } }; diff --git a/src/osd/scrubber/scrub_backend.cc b/src/osd/scrubber/scrub_backend.cc index 1f592a28ae9..2dfc4bc5f59 100644 --- a/src/osd/scrubber/scrub_backend.cc +++ b/src/osd/scrubber/scrub_backend.cc @@ -97,9 +97,10 @@ ScrubBackend::ScrubBackend(ScrubBeListener& scrubber, : (m_depth == scrub_level_t::deep ? "deep-scrub"sv : "scrub"sv)); } -uint64_t ScrubBackend::logical_to_ondisk_size(uint64_t logical_size) const +uint64_t ScrubBackend::logical_to_ondisk_size(uint64_t logical_size, + int8_t shard_id) const { - return m_pg.logical_to_ondisk_size(logical_size); + return m_pg.logical_to_ondisk_size(logical_size, shard_id); } void ScrubBackend::update_repair_status(bool should_repair) @@ -723,12 +724,12 @@ shard_as_auth_t ScrubBackend::possible_auth_shard(const hobject_t& obj, } } - if (test_error_cond(smap_obj.size != logical_to_ondisk_size(oi.size), - shard_info, + uint64_t ondisk_size = logical_to_ondisk_size(oi.size, srd.shard.id); + if (test_error_cond(smap_obj.size != ondisk_size, shard_info, &shard_info_wrapper::set_obj_size_info_mismatch)) { errstream << sep(err) << "candidate size " << smap_obj.size << " info size " - << logical_to_ondisk_size(oi.size) << " mismatch"; + << ondisk_size << " mismatch"; } std::optional digest; @@ -1290,8 +1291,8 @@ bool ScrubBackend::compare_obj_details(pg_shard_t auth_shard, // ------------------------------------------------------------------------ // sizes: - - uint64_t oi_size = logical_to_ondisk_size(auth_oi.size); + // NOTE: This will be fixed as a later PR as part of the optimized EC work. + uint64_t oi_size = logical_to_ondisk_size(auth_oi.size, 0); if (oi_size != candidate.size) { fmt::format_to(std::back_inserter(out), "{}size {} != size {} from auth oi {}", @@ -1468,12 +1469,13 @@ void ScrubBackend::scrub_snapshot_metadata(ScrubMap& map) } if (oi) { - if (logical_to_ondisk_size(oi->size) != p->second.size) { + // NOTE: Fix planned as part of the optimized EC work. + if (logical_to_ondisk_size(oi->size, 0) != p->second.size) { clog.error() << m_mode_desc << " " << m_pg_id << " " << soid << " : on disk size (" << p->second.size << ") does not match object info size (" << oi->size << ") adjusted for ondisk to (" - << logical_to_ondisk_size(oi->size) << ")"; + << logical_to_ondisk_size(oi->size, 0) << ")"; soid_error.set_size_mismatch(); this_chunk->m_error_counts.shallow_errors++; } diff --git a/src/osd/scrubber/scrub_backend.h b/src/osd/scrubber/scrub_backend.h index b42f416e8e7..01f46b985b8 100644 --- a/src/osd/scrubber/scrub_backend.h +++ b/src/osd/scrubber/scrub_backend.h @@ -517,7 +517,8 @@ class ScrubBackend { Scrub::SnapMapReaderI& snaps_getter); // accessing the PG backend for this translation service - uint64_t logical_to_ondisk_size(uint64_t logical_size) const; + uint64_t logical_to_ondisk_size(uint64_t logical_size, + int8_t shard_id) const; }; namespace fmt { diff --git a/src/osd/scrubber_common.h b/src/osd/scrubber_common.h index 4354998e2e1..917b99f98a1 100644 --- a/src/osd/scrubber_common.h +++ b/src/osd/scrubber_common.h @@ -279,7 +279,8 @@ struct PgScrubBeListener { virtual const pg_info_t& get_pg_info(ScrubberPasskey) const = 0; // query the PG backend for the on-disk size of an object - virtual uint64_t logical_to_ondisk_size(uint64_t logical_size) const = 0; + virtual uint64_t logical_to_ondisk_size(uint64_t logical_size, + int8_t shard_id) const = 0; // used to verify our "cleanliness" before scrubbing virtual bool is_waiting_for_unreadable_object() const = 0; diff --git a/src/test/crimson/test_crimson_scrub.cc b/src/test/crimson/test_crimson_scrub.cc index 1cd76988130..124edbd27bf 100644 --- a/src/test/crimson/test_crimson_scrub.cc +++ b/src/test/crimson/test_crimson_scrub.cc @@ -107,24 +107,6 @@ void so_mut_ss(ScrubMap::object &obj, F &&f) { so_set_ss(obj, std::invoke(std::forward(f), so_get_ss(obj))); } -void so_set_hinfo( - ScrubMap::object &obj, const std::optional &hinfo) -{ - return so_set_attr_type(obj, ECUtil::get_hinfo_key(), hinfo); -} - -std::optional so_get_hinfo(ScrubMap::object &obj) -{ - return so_get_attr_type(obj, ECUtil::get_hinfo_key()); -} - -template -void so_mut_hinfo(ScrubMap::object &obj, F &&f) { - auto maybe_hinfo = so_get_hinfo(obj); - auto new_maybe_hinfo = std::invoke(std::forward(f), std::move(maybe_hinfo)); - so_set_hinfo(obj, new_maybe_hinfo); -} - /** * so_builder_t * @@ -175,29 +157,8 @@ struct so_builder_t { return ret; } - static so_builder_t make_ec_head(std::string name) { - auto ret = make_head(name); - so_set_hinfo(ret.so, ECUtil::HashInfo{}); - return ret; - } - - static so_builder_t make_ec_clone( - std::string name, - snapid_t cloneid = 4 - ) { - auto ret = make_clone(name, cloneid); - so_set_hinfo(ret.so, ECUtil::HashInfo{}); - return ret; - } - - so_builder_t &set_size( - size_t size, - const std::optional stripe_info = std::nullopt) { - if (stripe_info) { - so.size = stripe_info->logical_to_next_chunk_offset(size); - } else { - so.size = size; - } + so_builder_t &set_size(size_t size) { + so.size = size; so_mut_oi(so, [size](auto maybe_oi) { if (maybe_oi) { @@ -205,14 +166,6 @@ struct so_builder_t { } return maybe_oi; }); - so_mut_hinfo(so, [size, &stripe_info](auto maybe_hinfo) { - if (maybe_hinfo) { - ceph_assert(stripe_info); - maybe_hinfo->set_total_chunk_size_clear_hash( - stripe_info->logical_to_next_chunk_offset(size)); - } - return maybe_hinfo; - }); return *this; } @@ -233,17 +186,14 @@ struct so_builder_t { * a stripe_info. */ struct test_obj_t : so_builder_t { - std::optional stripe_info; std::string desc; hobject_t hoid; test_obj_t( so_builder_t _builder, - std::optional _stripe_info, std::string _desc, hobject_t _hoid) : so_builder_t(std::move(_builder)), - stripe_info(std::move(_stripe_info)), desc(std::move(_desc)), hoid(std::move(_hoid)) { ceph_assert(!desc.empty()); @@ -251,12 +201,10 @@ struct test_obj_t : so_builder_t { static test_obj_t make( const std::string &desc, - std::optional stripe_info, so_builder_t builder) { hobject_t hoid = so_get_oi(builder.so)->soid; return test_obj_t{ std::move(builder), - stripe_info, desc, std::move(hoid)}; } @@ -265,7 +213,6 @@ struct test_obj_t : so_builder_t { static test_obj_t make_head(const std::string &desc, Args&&... args) { return make( desc, - std::nullopt, so_builder_t::make_head(std::forward(args)...)); } @@ -273,29 +220,11 @@ struct test_obj_t : so_builder_t { static test_obj_t make_clone(const std::string &desc, Args&&... args) { return make( desc, - std::nullopt, so_builder_t::make_clone(std::forward(args)...)); } - template - static test_obj_t make_ec_head(const std::string &desc, Args&&... args) { - return make( - desc, - ECUtil::stripe_info_t{4, 2, 1<<20}, - so_builder_t::make_ec_head(std::forward(args)...)); - } - - template - static test_obj_t make_ec_clone(const std::string &desc, Args&&... args) { - return make( - desc, - ECUtil::stripe_info_t{4, 2, 1<<20}, - so_builder_t::make_ec_clone(std::forward(args)...)); - } - - test_obj_t &set_size( - size_t size) { - so_builder_t::set_size(size, stripe_info); + test_obj_t &set_size(size_t size) { + so_builder_t::set_size(size); return *this; } @@ -556,44 +485,6 @@ struct CorruptSS : SingleErrorTestCaseT { } }; -struct MissingHinfo : SingleErrorTestCaseT { - constexpr static librados::err_t shard_error_sig{ - librados::err_t::HINFO_MISSING - }; - constexpr static librados::obj_err_t object_error_sig{ - librados::obj_err_t::HINFO_INCONSISTENCY - }; - constexpr static bool REQUIRES_EC = true; - - std::string_view get_description() const { - return "MissingHinfo"; - }; - test_obj_t inject_error(test_obj_t obj) const { - ceph_assert(obj.stripe_info); - so_mut_hinfo(obj.so, [](auto) { return std::nullopt; }); - return obj; - } -}; - -struct CorruptHinfo : SingleErrorTestCaseT { - constexpr static librados::err_t shard_error_sig{ - librados::err_t::HINFO_CORRUPTED - }; - constexpr static librados::obj_err_t object_error_sig{ - librados::obj_err_t::HINFO_INCONSISTENCY - }; - constexpr static bool REQUIRES_EC = true; - - std::string_view get_description() const { - return "CorruptHinfo"; - }; - test_obj_t inject_error(test_obj_t obj) const { - ceph_assert(obj.stripe_info); - so_set_attr_len(obj.so, ECUtil::get_hinfo_key(), 10); - return obj; - } -}; - struct DataDigestMismatch : SingleErrorTestCaseT { constexpr static librados::err_t shard_error_sig{ librados::err_t::DATA_DIGEST_MISMATCH_INFO @@ -707,8 +598,6 @@ std::unique_ptr test_cases[] = { std::make_unique(), std::make_unique(), std::make_unique(), - std::make_unique(), - std::make_unique(), std::make_unique(), std::make_unique(), std::make_unique(), @@ -804,7 +693,6 @@ TEST_P(TestSingleError, SingleError) { const pg_shard_t replica(1, shard_id_t::NO_SHARD); crimson::osd::scrub::chunk_validation_policy_t policy { primary, - obj.stripe_info, TEST_MAX_OBJECT_SIZE, std::string{TEST_INTERNAL_NAMESPACE}, TEST_OMAP_KEY_LIMIT, @@ -883,9 +771,7 @@ INSTANTIATE_TEST_SUITE_P( ::testing::Values( test_obj_t::make_head("Small", "foo").set_size(64), test_obj_t::make_clone("EmptyWithAttr", "foo2").add_attr("extra_attr", 64), - test_obj_t::make_head("ReplicatedRBD", "foo2").set_size(4<<20), - test_obj_t::make_ec_head("ECHead", "foo").set_size(4<<20), - test_obj_t::make_ec_clone("LargeECClone", "foo").set_size(16<<20) + test_obj_t::make_head("ReplicatedRBD", "foo2").set_size(4<<20) ), ::testing::Bool(), ::testing::ValuesIn( @@ -907,9 +793,7 @@ INSTANTIATE_TEST_SUITE_P( ::testing::Values( test_obj_t::make_head("Small", "foo").set_size(64), test_obj_t::make_clone("EmptyWithAttr", "foo2").add_attr("extra_attr", 64), - test_obj_t::make_head("ReplicatedRBD", "foo2").set_size(4<<20), - test_obj_t::make_ec_head("ECHead", "foo").set_size(4<<20), - test_obj_t::make_ec_clone("LargeECClone", "foo").set_size(16<<20) + test_obj_t::make_head("ReplicatedRBD", "foo2").set_size(4<<20) ), ::testing::Values(false), // replica only ::testing::ValuesIn( @@ -921,25 +805,6 @@ INSTANTIATE_TEST_SUITE_P( } ); -/* Some tests only make sense on ec objects. */ -INSTANTIATE_TEST_SUITE_P( - SingleErrorOnly, - TestSingleError, - ::testing::Combine( - ::testing::Values( - test_obj_t::make_ec_head("ECHead", "foo").set_size(4<<20), - test_obj_t::make_ec_clone("LargeECClone", "foo").set_size(16<<20) - ), - ::testing::Bool(), - ::testing::ValuesIn( - test_cases_begin(), - test_cases_end()) - ), - [](const auto &info) { - return fmt::format("{}", info.param); - } -); - /* Some tests only make sense on head objects. */ INSTANTIATE_TEST_SUITE_P( SingleErrorHEAD, @@ -947,8 +812,7 @@ INSTANTIATE_TEST_SUITE_P( ::testing::Combine( ::testing::Values( test_obj_t::make_head("Small", "foo").set_size(64), - test_obj_t::make_head("ReplicatedRBD", "foo2").set_size(4<<20), - test_obj_t::make_ec_head("ECHead", "foo").set_size(4<<20) + test_obj_t::make_head("ReplicatedRBD", "foo2").set_size(4<<20) ), ::testing::Bool(), ::testing::ValuesIn( @@ -1100,7 +964,6 @@ TEST_P(TestSnapSetCloneError, CloneError) { const pg_shard_t primary(0, shard_id_t::NO_SHARD); crimson::osd::scrub::chunk_validation_policy_t policy { primary, - std::nullopt, TEST_MAX_OBJECT_SIZE, std::string{TEST_INTERNAL_NAMESPACE}, TEST_OMAP_KEY_LIMIT, @@ -1184,7 +1047,6 @@ TEST(TestSnapSet, MissingHead) { const pg_shard_t primary(0, shard_id_t::NO_SHARD); crimson::osd::scrub::chunk_validation_policy_t policy { primary, - std::nullopt, TEST_MAX_OBJECT_SIZE, std::string{TEST_INTERNAL_NAMESPACE}, TEST_OMAP_KEY_LIMIT, @@ -1220,7 +1082,6 @@ TEST(TestSnapSet, Stats) { const pg_shard_t primary(0, shard_id_t::NO_SHARD); crimson::osd::scrub::chunk_validation_policy_t policy { primary, - std::nullopt, TEST_MAX_OBJECT_SIZE, std::string{TEST_INTERNAL_NAMESPACE}, TEST_OMAP_KEY_LIMIT, diff --git a/src/test/osd/test_scrubber_be.cc b/src/test/osd/test_scrubber_be.cc index 65fd7e730ba..7afb5c4ba7a 100644 --- a/src/test/osd/test_scrubber_be.cc +++ b/src/test/osd/test_scrubber_be.cc @@ -90,7 +90,8 @@ class TestPg : public PgScrubBeListener { const pg_info_t& get_pg_info(ScrubberPasskey) const final { return m_info; } - uint64_t logical_to_ondisk_size(uint64_t logical_size) const final + uint64_t logical_to_ondisk_size(uint64_t logical_size, + int8_t shard_id) const final { return logical_size; } -- 2.39.5