From d9a1b54e1990558c7e2ec55ba1df8571aa6b4dbe Mon Sep 17 00:00:00 2001 From: Ronen Friedman Date: Sun, 24 Jul 2022 13:25:55 +0000 Subject: [PATCH] osd/scrub: improve SnapMapper's API used by the scrubber By: - defining the interface; - avoiding 'out' parameters where possible - (forced to) improved const correctness Signed-off-by: Ronen Friedman --- src/osd/SnapMapReaderI.h | 68 ++++++++++++ src/osd/SnapMapper.cc | 181 ++++++++++++++++++++++++++----- src/osd/SnapMapper.h | 57 ++++++++-- src/osd/scrubber/scrub_backend.h | 43 +------- src/test/osd/test_scrubber_be.cc | 28 ++++- 5 files changed, 300 insertions(+), 77 deletions(-) create mode 100644 src/osd/SnapMapReaderI.h diff --git a/src/osd/SnapMapReaderI.h b/src/osd/SnapMapReaderI.h new file mode 100644 index 00000000000..f979dff5db2 --- /dev/null +++ b/src/osd/SnapMapReaderI.h @@ -0,0 +1,68 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab +#pragma once + +/** + * \file + * \brief Defines the interface for the snap-mapper used by the scrubber. + */ +#include + +#include "common/scrub_types.h" +#include "include/expected.hpp" + +namespace Scrub { +/* + * snaps-related aux structures: + * the scrub-backend scans the snaps associated with each scrubbed object, and + * fixes corrupted snap-sets. + * The actual access to the PG's snap_mapper, and the actual I/O transactions, + * are performed by the main PgScrubber object. + * the following aux structures are used to facilitate the required exchanges: + * - pre-fix snap-sets are accessed by the scrub-backend, and: + * - a list of fix-orders (either insert or replace operations) are returned + */ + +struct SnapMapReaderI { + struct result_t { + enum class code_t { success, backend_error, not_found, inconsistent }; + code_t code{code_t::success}; + int backend_error{0}; ///< errno returned by the backend + }; + + /** + * get SnapMapper's snap-set for a given object + * \returns a set of snaps, or an error code + * \attn: only OBJ_ DB entries are consulted + */ + virtual tl::expected, result_t> get_snaps( + const hobject_t& hoid) const = 0; + + /** + * get SnapMapper's snap-set for a given object. + * The snaps gleaned from the OBJ_ entry are verified against the + * mapping ('SNA_') entries. + * A mismatch between both sets of entries will result in an error. + * \returns a set of snaps, or an error code. + */ + virtual tl::expected, result_t> + get_snaps_check_consistency(const hobject_t& hoid) const = 0; + + virtual ~SnapMapReaderI() = default; +}; + +} // namespace Scrub + +enum class snap_mapper_op_t { + add, + update, + overwrite, //< the mapper's data is internally inconsistent. Similar + //< to an 'update' operation, but the logs are different. +}; + +struct snap_mapper_fix_t { + snap_mapper_op_t op; + hobject_t hoid; + std::set snaps; + std::set wrong_snaps; // only collected & returned for logging sake +}; diff --git a/src/osd/SnapMapper.cc b/src/osd/SnapMapper.cc index 9936808c10d..b943c1e5d71 100644 --- a/src/osd/SnapMapper.cc +++ b/src/osd/SnapMapper.cc @@ -13,7 +13,12 @@ */ #include "SnapMapper.h" + #include +#include + +#include "osd/osd_types_fmt.h" +#include "SnapMapReaderI.h" #define dout_context cct #define dout_subsys ceph_subsys_osd @@ -31,6 +36,9 @@ using ceph::bufferlist; using ceph::decode; using ceph::encode; using ceph::timespan_str; +using result_t = Scrub::SnapMapReaderI::result_t; +using code_t = Scrub::SnapMapReaderI::result_t::code_t; + const string SnapMapper::LEGACY_MAPPING_PREFIX = "MAP_"; const string SnapMapper::MAPPING_PREFIX = "SNA_"; @@ -108,19 +116,22 @@ string SnapMapper::get_prefix(int64_t pool, snapid_t snap) } string SnapMapper::to_raw_key( - const pair &in) + const pair &in) const { return get_prefix(in.second.pool, in.first) + shard_prefix + in.second.to_str(); } +std::string SnapMapper::to_raw_key(snapid_t snap, const hobject_t &clone) const +{ + return get_prefix(clone.pool, snap) + shard_prefix + clone.to_str(); +} + pair SnapMapper::to_raw( - const pair &in) + const pair &in) const { bufferlist bl; encode(Mapping(in), bl); - return make_pair( - to_raw_key(in), - bl); + return make_pair(to_raw_key(in), bl); } pair SnapMapper::from_raw( @@ -134,12 +145,22 @@ pair SnapMapper::from_raw( return make_pair(map.snap, map.hoid); } +std::pair SnapMapper::from_raw( + const ceph::buffer::list &image) +{ + using ceph::decode; + Mapping map; + auto bp = image.cbegin(); + decode(map, bp); + return make_pair(map.snap, map.hoid); +} + bool SnapMapper::is_mapping(const string &to_test) { return to_test.substr(0, MAPPING_PREFIX.size()) == MAPPING_PREFIX; } -string SnapMapper::to_object_key(const hobject_t &hoid) +string SnapMapper::to_object_key(const hobject_t &hoid) const { return OBJECT_PREFIX + shard_prefix + hoid.to_str(); } @@ -171,35 +192,141 @@ bool SnapMapper::check(const hobject_t &hoid) const return false; } -int SnapMapper::get_snaps( - const hobject_t &oid, - object_snaps *out) +int SnapMapper::get_snaps(const hobject_t &oid, object_snaps *out) const +{ + auto snaps = get_snaps_common(oid); + if (snaps) { + *out = *snaps; + return 0; + } + switch (auto e = snaps.error(); e.code) { + case code_t::backend_error: + return e.backend_error; + case code_t::not_found: + return -ENOENT; + case code_t::inconsistent: + // As this is a legacy interface, we cannot surprise the user with + // a new error code here. + return -ENOENT; + default: + // Can't happen. Just to keep the compiler happy. + ceph_abort("get_snaps_common() returned invalid error code"); + } +} + +tl::expected, Scrub::SnapMapReaderI::result_t> +SnapMapper::get_snaps(const hobject_t &oid) const +{ + auto snaps = get_snaps_common(oid); + if (snaps) { + return snaps->snaps; + } + return tl::unexpected(snaps.error()); +} + +tl::expected +SnapMapper::get_snaps_common(const hobject_t &oid) const { ceph_assert(check(oid)); - set keys; + set keys{to_object_key(oid)}; + dout(20) << fmt::format("{}: key string: {} oid:{}", __func__, keys, oid) + << dendl; + map got; - keys.insert(to_object_key(oid)); int r = backend.get_keys(keys, &got); if (r < 0) { - dout(20) << __func__ << " " << oid << " got err " << r << dendl; - return r; + dout(10) << __func__ << " " << oid << " got err " << r << dendl; + return tl::unexpected(result_t{code_t::backend_error, r}); } if (got.empty()) { - dout(20) << __func__ << " " << oid << " got.empty()" << dendl; - return -ENOENT; + dout(10) << __func__ << " " << oid << " got.empty()" << dendl; + return tl::unexpected(result_t{code_t::not_found, -ENOENT}); } - if (out) { - auto bp = got.begin()->second.cbegin(); - decode(*out, bp); - dout(20) << __func__ << " " << oid << " " << out->snaps << dendl; - if (out->snaps.empty()) { - dout(1) << __func__ << " " << oid << " empty snapset" << dendl; - ceph_assert(!cct->_conf->osd_debug_verify_snaps); + + object_snaps out; + auto bp = got.begin()->second.cbegin(); + try { + decode(out, bp); + } catch (...) { + dout(1) << __func__ << ": " << oid << " decode failed" << dendl; + return tl::unexpected(result_t{code_t::backend_error, -EIO}); + } + + dout(20) << __func__ << " " << oid << " " << out.snaps << dendl; + if (out.snaps.empty()) { + dout(1) << __func__ << " " << oid << " empty snapset" << dendl; + ceph_assert(!cct->_conf->osd_debug_verify_snaps); + } + return out; +} + +std::set SnapMapper::to_raw_keys( + const hobject_t &clone, + const std::set &snaps) const +{ + std::set keys; + for (auto snap : snaps) { + keys.insert(to_raw_key(snap, clone)); + } + dout(20) << fmt::format( + "{}: clone:{} snaps:{} -> keys: {}", __func__, clone, snaps, + keys) + << dendl; + return keys; +} + +tl::expected, result_t> +SnapMapper::get_snaps_check_consistency(const hobject_t &hoid) const +{ + // derive the set of snaps from the 'OBJ_' entry + auto obj_snaps = get_snaps(hoid); + if (!obj_snaps) { + return obj_snaps; + } + + // make sure we have the expected set of SNA_ entries: + // we have the clone oid and the set of snaps relevant to this clone. + // Let's construct all expected SNA_ key, then fetch them. + + map kvmap; + auto mapping_keys = to_raw_keys(hoid, *obj_snaps); + auto r = backend.get_keys(mapping_keys, &kvmap); + if (r < 0) { + dout(10) << fmt::format( + "{}: backend error ({}) for cobject {}", __func__, r, hoid) + << dendl; + // that's a backend error, but for the SNA_ entries. Let's treat it as an + // internal consistency error (although a backend error would have made + // sense too). + return tl::unexpected(result_t{code_t::inconsistent, r}); + } + + std::set snaps_from_mapping; + for (auto &[k, v] : kvmap) { + dout(20) << __func__ << " " << hoid << " " << k << dendl; + // extract the object ID from the value fetched for an SNA mapping key + auto [sn, obj] = SnapMapper::from_raw(v); + if (obj != hoid) { + dout(1) << fmt::format( + "{}: unexpected object ID {} for key{} (expected {})", + __func__, obj, k, hoid) + << dendl; + return tl::unexpected(result_t{code_t::inconsistent}); } - } else { - dout(20) << __func__ << " " << oid << " (out == NULL)" << dendl; + snaps_from_mapping.insert(sn); } - return 0; + + if (snaps_from_mapping != *obj_snaps) { + dout(10) << fmt::format( + "{}: hoid:{} -> mapper internal inconsistency ({} vs {})", + __func__, hoid, *obj_snaps, snaps_from_mapping) + << dendl; + return tl::unexpected(result_t{code_t::inconsistent}); + } + dout(10) << fmt::format( + "{}: snaps for {}: {}", __func__, hoid, snaps_from_mapping) + << dendl; + return obj_snaps; } void SnapMapper::clear_snaps( @@ -326,6 +453,8 @@ int SnapMapper::get_next_objects_to_trim( // trim the snaptrim queue ceph_assert(max > 0); int r = 0; + + /// \todo cache the prefixes-set in update_bits() for (set::iterator i = prefixes.begin(); i != prefixes.end() && out->size() < max && r == 0; ++i) { @@ -402,7 +531,7 @@ int SnapMapper::_remove_oid( int SnapMapper::get_snaps( const hobject_t &oid, - std::set *snaps) + std::set *snaps) const { ceph_assert(check(oid)); object_snaps out; diff --git a/src/osd/SnapMapper.h b/src/osd/SnapMapper.h index 90b0c7c8d82..36eed57810c 100644 --- a/src/osd/SnapMapper.h +++ b/src/osd/SnapMapper.h @@ -15,18 +15,19 @@ #ifndef SNAPMAPPER_H #define SNAPMAPPER_H -#include +#include #include +#include #include -#include -#include "common/map_cacher.hpp" #include "common/hobject.h" +#include "common/map_cacher.hpp" #include "include/buffer.h" #include "include/encoding.h" #include "include/object.h" #include "os/ObjectStore.h" #include "osd/OSDMap.h" +#include "osd/SnapMapReaderI.h" class OSDriver : public MapCacher::StoreDriver { ObjectStore *os; @@ -64,6 +65,11 @@ public: return OSTransaction(ch->cid, hoid, t); } + OSTransaction get_transaction( + ObjectStore::Transaction *t) const { + return OSTransaction(ch->cid, hoid, t); + } + OSDriver(ObjectStore *os, const coll_t& cid, const ghobject_t &hoid) : os(os), hoid(hoid) { @@ -99,7 +105,7 @@ public: * snap will sort together, and so that all objects in a pg for a * particular snap will group under up to 8 prefixes. */ -class SnapMapper { +class SnapMapper : public Scrub::SnapMapReaderI { friend class MapperVerifier; public: CephContext* cct; @@ -213,8 +219,9 @@ private: snapid_t end, std::map *m); static std::string make_purged_snap_key(int64_t pool, snapid_t last); - - MapCacher::MapCacher backend; + // note: marked 'mutable', as functions as a cache and used in some 'const' + // functions. + mutable MapCacher::MapCacher backend; static std::string get_legacy_prefix(snapid_t snap); std::string to_legacy_raw_key( @@ -223,19 +230,28 @@ private: static std::string get_prefix(int64_t pool, snapid_t snap); std::string to_raw_key( - const std::pair &to_map); + const std::pair &to_map) const; + + std::string to_raw_key(snapid_t snap, const hobject_t& clone) const; std::pair to_raw( - const std::pair &to_map); + const std::pair &to_map) const; static bool is_mapping(const std::string &to_test); static std::pair from_raw( const std::pair &image); - std::string to_object_key(const hobject_t &hoid); + static std::pair from_raw( + const ceph::buffer::list& image); - int get_snaps(const hobject_t &oid, object_snaps *out); + std::string to_object_key(const hobject_t &hoid) const; + + int get_snaps(const hobject_t &oid, object_snaps *out) const; + + std::set to_raw_keys( + const hobject_t &clone, + const std::set &snaps) const; void set_snaps( const hobject_t &oid, @@ -254,7 +270,11 @@ private: MapCacher::Transaction *t ///< [out] transaction ); -public: + /// Get snaps (as an 'object_snaps' object) for oid + tl::expected get_snaps_common( + const hobject_t &hoid) const; + + public: static std::string make_shard_prefix(shard_id_t shard) { if (shard == shard_id_t::NO_SHARD) return std::string(); @@ -330,7 +350,20 @@ public: int get_snaps( const hobject_t &oid, ///< [in] oid to get snaps for std::set *snaps ///< [out] snaps - ); ///< @return error, -ENOENT if oid is not recorded + ) const; ///< @return error, -ENOENT if oid is not recorded + + /// Get snaps for oid - alternative interface + tl::expected, SnapMapReaderI::result_t> get_snaps( + const hobject_t &hoid) const final; + + /** + * get_snaps_check_consistency + * + * Returns snaps for hoid as in get_snaps(), but additionally validates the + * snap->hobject_t mappings ('SNA_' entries). + */ + tl::expected, SnapMapReaderI::result_t> + get_snaps_check_consistency(const hobject_t &hoid) const final; }; WRITE_CLASS_ENCODER(SnapMapper::object_snaps) WRITE_CLASS_ENCODER(SnapMapper::Mapping) diff --git a/src/osd/scrubber/scrub_backend.h b/src/osd/scrubber/scrub_backend.h index 81259e5fb81..e8c7c2bcabb 100644 --- a/src/osd/scrubber/scrub_backend.h +++ b/src/osd/scrubber/scrub_backend.h @@ -47,10 +47,9 @@ #include "common/LogClient.h" #include "osd/OSDMap.h" -#include "common/scrub_types.h" #include "osd/osd_types_fmt.h" - #include "osd/scrubber_common.h" +#include "osd/SnapMapReaderI.h" struct ScrubMap; @@ -99,37 +98,7 @@ struct ScrubBeListener { virtual ~ScrubBeListener() = default; }; - -/* - * snaps-related aux structures: - * the scrub-backend scans the snaps associated with each scrubbed object, and - * fixes corrupted snap-sets. - * The actual access to the PG's snap_mapper, and the actual I/O transactions, - * are performed by the main PgScrubber object. - * the following aux structures are used to facilitate the required exchanges: - * - pre-fix snap-sets are accessed by the scrub-backend, and: - * - a list of fix-orders (either insert or replace operations) are returned - */ - -struct SnapMapperAccessor { - virtual int get_snaps(const hobject_t& hoid, - std::set* snaps_set) const = 0; - virtual ~SnapMapperAccessor() = default; -}; - -enum class snap_mapper_op_t { - add, - update, -}; - -struct snap_mapper_fix_t { - snap_mapper_op_t op; - hobject_t hoid; - std::set snaps; - std::set wrong_snaps; // only collected & returned for logging sake -}; - -// and - as the main scrub-backend entry point - scrub_compare_maps() - must +// As the main scrub-backend entry point - scrub_compare_maps() - must // be able to return both a list of snap fixes and a list of inconsistent // objects: struct objs_fix_list_t { @@ -356,7 +325,7 @@ class ScrubBackend { ScrubMap& smap, bool max_reached, const hobject_t& start, - SnapMapperAccessor& snaps_getter); + Scrub::SnapMapReaderI& snaps_getter); /** * decode the arriving MOSDRepScrubMap message, placing the replica's @@ -367,7 +336,7 @@ class ScrubBackend { void decode_received_map(pg_shard_t from, const MOSDRepScrubMap& msg); objs_fix_list_t scrub_compare_maps(bool max_reached, - SnapMapperAccessor& snaps_getter); + Scrub::SnapMapReaderI& snaps_getter); int scrub_process_inconsistent(); @@ -533,7 +502,7 @@ class ScrubBackend { */ std::vector scan_snaps( ScrubMap& smap, - SnapMapperAccessor& snaps_getter); + Scrub::SnapMapReaderI& snaps_getter); /** * an aux used by scan_snaps(), possibly returning a fix-order @@ -542,7 +511,7 @@ class ScrubBackend { std::optional scan_object_snaps( const hobject_t& hoid, const SnapSet& snapset, - SnapMapperAccessor& snaps_getter); + Scrub::SnapMapReaderI& snaps_getter); // accessing the PG backend for this translation service uint64_t logical_to_ondisk_size(uint64_t logical_size) const; diff --git a/src/test/osd/test_scrubber_be.cc b/src/test/osd/test_scrubber_be.cc index 1c7f0abab65..fb8518de6a8 100644 --- a/src/test/osd/test_scrubber_be.cc +++ b/src/test/osd/test_scrubber_be.cc @@ -107,7 +107,8 @@ class TestPg : public PgScrubBeListener { // /////////////////////////////////////////////////////////////////////////// // and the scrubber -class TestScrubber : public ScrubBeListener, public SnapMapperAccessor { +class TestScrubber : public ScrubBeListener, public Scrub::SnapMapReaderI { + using result_t = Scrub::SnapMapReaderI::result_t; public: ~TestScrubber() = default; @@ -143,7 +144,17 @@ class TestScrubber : public ScrubBeListener, public SnapMapperAccessor { } int get_snaps(const hobject_t& hoid, - std::set* snaps_set) const final; + std::set* snaps_set) const; + + tl::expected, result_t> get_snaps( + const hobject_t& oid) const final; + + tl::expected, result_t> get_snaps_check_consistency( + const hobject_t& oid) const final + { + /// \todo for now + return get_snaps(oid); + } void set_snaps(const hobject_t& hoid, const std::vector& snaps) { @@ -198,6 +209,19 @@ int TestScrubber::get_snaps(const hobject_t& hoid, return 0; } +tl::expected, Scrub::SnapMapReaderI::result_t> +TestScrubber::get_snaps(const hobject_t& oid) const +{ + std::set snapset; + auto r = get_snaps(oid, &snapset); + if (r >= 0) { + return snapset; + } + return tl::make_unexpected(Scrub::SnapMapReaderI::result_t{ + Scrub::SnapMapReaderI::result_t::code_t::not_found, + r}); +} + // /////////////////////////////////////////////////////////////////////////// // /////////////////////////////////////////////////////////////////////////// -- 2.39.5