diff --git a/src/common/scrub_types.cc b/src/common/scrub_types.cc index 9168ee0a27939..b03a3cab70c84 100644 --- a/src/common/scrub_types.cc +++ b/src/common/scrub_types.cc @@ -55,10 +55,12 @@ static void encode(const osd_shard_t& shard, bufferlist& bl) { void shard_info_wrapper::set_object(const ScrubMap::object& object) { - for (auto attr : object.attrs) { - bufferlist bl; - bl.push_back(attr.second); - attrs.insert(std::make_pair(attr.first, std::move(bl))); + // logically no-op, changes the comparator from std::less + // while avoiding `reinterpret_cast&>(object.attrs)` + attrs.clear(); + for (const auto& kv : object.attrs) { + attrs.insert(kv); } size = object.size; if (object.omap_digest_present) { diff --git a/src/crimson/osd/osd_operations/scrub_events.cc b/src/crimson/osd/osd_operations/scrub_events.cc index 4f54cf0b274f3..a488077d6a77f 100644 --- a/src/crimson/osd/osd_operations/scrub_events.cc +++ b/src/crimson/osd/osd_operations/scrub_events.cc @@ -220,7 +220,7 @@ ScrubScan::ifut<> ScrubScan::scan_object( if (i.second.length() == 0) { entry.attrs[i.first]; } else { - entry.attrs.emplace(i.first, i.second.front()); + entry.attrs.emplace(i.first, i.second); } } }).handle_error_interruptible( diff --git a/src/crimson/osd/scrub/scrub_validator.cc b/src/crimson/osd/scrub/scrub_validator.cc index b7dcc46a35e7c..a3979f790bca4 100644 --- a/src/crimson/osd/scrub/scrub_validator.cc +++ b/src/crimson/osd/scrub/scrub_validator.cc @@ -96,11 +96,9 @@ shard_evaluation_t evaluate_object_shard( if (xiter == obj.attrs.end()) { ret.shard_info.set_info_missing(); } else { - bufferlist bl; - bl.push_back(xiter->second); ret.object_info = object_info_t{}; try { - auto bliter = bl.cbegin(); + auto bliter = xiter->second.cbegin(); ::decode(*(ret.object_info), bliter); } catch (...) { ret.shard_info.set_info_corrupted(); @@ -120,11 +118,9 @@ shard_evaluation_t evaluate_object_shard( if (xiter == obj.attrs.end()) { ret.shard_info.set_snapset_missing(); } else { - bufferlist bl; - bl.push_back(xiter->second); ret.snapset = SnapSet{}; try { - auto bliter = bl.cbegin(); + auto bliter = xiter->second.cbegin(); ::decode(*(ret.snapset), bliter); } catch (...) { ret.shard_info.set_snapset_corrupted(); @@ -138,11 +134,9 @@ shard_evaluation_t evaluate_object_shard( if (xiter == obj.attrs.end()) { ret.shard_info.set_hinfo_missing(); } else { - bufferlist bl; - bl.push_back(xiter->second); ret.hinfo = ECUtil::HashInfo{}; try { - auto bliter = bl.cbegin(); + auto bliter = xiter->second.cbegin(); decode(*(ret.hinfo), bliter); } catch (...) { ret.shard_info.set_hinfo_corrupted(); diff --git a/src/osd/ECBackend.cc b/src/osd/ECBackend.cc index 1322f7f47f315..836bf96dde445 100644 --- a/src/osd/ECBackend.cc +++ b/src/osd/ECBackend.cc @@ -518,7 +518,7 @@ void ECBackend::continue_recovery_op( if (op.recovery_progress.first && op.obc) { /* We've got the attrs and the hinfo, might as well use them */ - op.hinfo = get_hash_info(op.hoid); + op.hinfo = get_hash_info(op.hoid, false, &op.obc->attr_cache); if (!op.hinfo) { derr << __func__ << ": " << op.hoid << " has inconsistent hinfo" << dendl; @@ -1012,7 +1012,14 @@ void ECBackend::handle_sub_read( // Do NOT check osd_read_eio_on_bad_digest here. We need to report // the state of our chunk in case other chunks could substitute. ECUtil::HashInfoRef hinfo; - hinfo = get_hash_info(i->first); + map> attrs; + int r = PGBackend::objects_get_attrs(i->first, &attrs); + if (r >= 0) { + hinfo = get_hash_info(i->first, false, &attrs); + } else { + derr << "get_hash_info" << ": stat " << i->first << " failed: " + << cpp_strerror(r) << dendl; + } if (!hinfo) { r = -EIO; get_parent()->clog_error() << "Corruption detected: object " @@ -1370,7 +1377,7 @@ void ECBackend::submit_transaction( sinfo, *(op->t), [&](const hobject_t &i) { - ECUtil::HashInfoRef ref = get_hash_info(i, true); + ECUtil::HashInfoRef ref = get_hash_info(i, true, &op->t->obc_map[hoid]->attr_cache); if (!ref) { derr << __func__ << ": get_hash_info(" << i << ")" << " returned a null pointer and there is no " @@ -1387,7 +1394,7 @@ void ECBackend::submit_transaction( ECUtil::HashInfoRef ECBackend::get_hash_info( - const hobject_t &hoid, bool create, const map> *attrs) + const hobject_t &hoid, bool create, const map> *attrs) { dout(10) << __func__ << ": Getting attr on " << hoid << dendl; ECUtil::HashInfoRef ref = unstable_hashinfo_registry.lookup(hoid); @@ -1402,23 +1409,12 @@ ECUtil::HashInfoRef ECBackend::get_hash_info( if (r >= 0) { dout(10) << __func__ << ": found on disk, size " << st.st_size << dendl; bufferlist bl; - if (attrs) { - map::const_iterator k = attrs->find(ECUtil::get_hinfo_key()); - if (k == attrs->end()) { - dout(5) << __func__ << " " << hoid << " missing hinfo attr" << dendl; - } else { - bl.push_back(k->second); - } + ceph_assert(attrs); + map::const_iterator k = attrs->find(ECUtil::get_hinfo_key()); + if (k == attrs->end()) { + dout(5) << __func__ << " " << hoid << " missing hinfo attr" << dendl; } else { - r = store->getattr( - ch, - ghobject_t(hoid, ghobject_t::NO_GEN, get_parent()->whoami_shard().shard), - ECUtil::get_hinfo_key(), - bl); - if (r < 0) { - dout(5) << __func__ << ": getattr failed: " << cpp_strerror(r) << dendl; - bl.clear(); // just in case - } + bl = k->second; } if (bl.length() > 0) { auto bp = bl.cbegin(); @@ -1596,10 +1592,8 @@ int ECBackend::objects_get_attrs( const hobject_t &hoid, map> *out) { - int r = store->getattrs( - ch, - ghobject_t(hoid, ghobject_t::NO_GEN, get_parent()->whoami_shard().shard), - *out); + // call from parents -- get raw attrs, without any filtering for hinfo + int r = PGBackend::objects_get_attrs(hoid, out); if (r < 0) return r; diff --git a/src/osd/ECBackend.h b/src/osd/ECBackend.h index 54b8e16119ebf..27176bf18f927 100644 --- a/src/osd/ECBackend.h +++ b/src/osd/ECBackend.h @@ -325,8 +325,8 @@ class ECBackend : public PGBackend, public ECCommon { const ECUtil::stripe_info_t sinfo; /// If modified, ensure that the ref is held until the update is applied SharedPtrRegistry unstable_hashinfo_registry; - ECUtil::HashInfoRef get_hash_info(const hobject_t &hoid, bool create = false, - const std::map> *attr = NULL); + ECUtil::HashInfoRef get_hash_info(const hobject_t &hoid, bool create, + const std::map> *attr); public: ECBackend( diff --git a/src/osd/osd_types.cc b/src/osd/osd_types.cc index 7596723a0e30d..1a6b6041d66fe 100644 --- a/src/osd/osd_types.cc +++ b/src/osd/osd_types.cc @@ -7148,8 +7148,16 @@ void ScrubMap::object::generate_test_instances(list& o) o.back()->negative = true; o.push_back(new object); o.back()->size = 123; - o.back()->attrs["foo"] = ceph::buffer::copy("foo", 3); - o.back()->attrs["bar"] = ceph::buffer::copy("barval", 6); + { + bufferlist foobl; + foobl.push_back(ceph::buffer::copy("foo", 3)); + o.back()->attrs["foo"] = std::move(foobl); + } + { + bufferlist barbl; + barbl.push_back(ceph::buffer::copy("barval", 6)); + o.back()->attrs["bar"] = std::move(barbl); + } } // -- OSDOp -- diff --git a/src/osd/osd_types.h b/src/osd/osd_types.h index ff02ca5730f78..a8e6b5994a93b 100644 --- a/src/osd/osd_types.h +++ b/src/osd/osd_types.h @@ -6233,7 +6233,7 @@ std::ostream& operator<<(std::ostream& out, const PushOp &op); */ struct ScrubMap { struct object { - std::map> attrs; + std::map> attrs; uint64_t size; __u32 omap_digest; ///< omap crc32c __u32 digest; ///< data crc32c diff --git a/src/osd/osd_types_fmt.h b/src/osd/osd_types_fmt.h index d6d746d295f75..6ffe95a3e95fc 100644 --- a/src/osd/osd_types_fmt.h +++ b/src/osd/osd_types_fmt.h @@ -275,15 +275,13 @@ struct fmt::formatter { // note the special handling of (1) OI_ATTR and (2) non-printables for (auto [k, v] : so.attrs) { - std::string bkstr{v.raw_c_str(), v.raw_length()}; + std::string bkstr = v.to_str(); if (k == std::string{OI_ATTR}) { /// \todo consider parsing the OI args here. Maybe add a specific format /// specifier fmt::format_to(ctx.out(), "{{{}:<>({})}} ", k, bkstr.length()); } else if (k == std::string{SS_ATTR}) { - bufferlist bl; - bl.push_back(v); - SnapSet sns{bl}; + SnapSet sns{v}; fmt::format_to(ctx.out(), "{{{}:{:D}}} ", k, sns); } else { fmt::format_to(ctx.out(), "{{{}:{}({})}} ", k, bkstr, bkstr.length()); diff --git a/src/osd/scrubber/pg_scrubber.cc b/src/osd/scrubber/pg_scrubber.cc index 2dae53273a705..3bfc3a10f10a4 100644 --- a/src/osd/scrubber/pg_scrubber.cc +++ b/src/osd/scrubber/pg_scrubber.cc @@ -1321,11 +1321,9 @@ void PgScrubber::repair_oinfo_oid(ScrubMap& smap) if (o.attrs.find(OI_ATTR) == o.attrs.end()) { continue; } - bufferlist bl; - bl.push_back(o.attrs[OI_ATTR]); object_info_t oi; try { - oi.decode(bl); + oi.decode(o.attrs[OI_ATTR]); } catch (...) { continue; } @@ -1340,13 +1338,12 @@ void PgScrubber::repair_oinfo_oid(ScrubMap& smap) << "...repaired"; // Fix object info oi.soid = hoid; - bl.clear(); + bufferlist bl; encode(oi, bl, m_pg->get_osdmap()->get_features(CEPH_ENTITY_TYPE_OSD, nullptr)); - bufferptr bp(bl.c_str(), bl.length()); - o.attrs[OI_ATTR] = bp; + o.attrs[OI_ATTR] = std::move(bl); t.setattr(m_pg->coll, ghobject_t(hoid), OI_ATTR, bl); int r = m_pg->osd->store->queue_transaction(m_pg->ch, std::move(t)); diff --git a/src/osd/scrubber/scrub_backend.cc b/src/osd/scrubber/scrub_backend.cc index e25c5b99da09c..2d7d1a4ecf174 100644 --- a/src/osd/scrubber/scrub_backend.cc +++ b/src/osd/scrubber/scrub_backend.cc @@ -376,7 +376,7 @@ void ScrubBackend::repair_object(const hobject_t& soid, try { bufferlist bv; if (po.attrs.count(OI_ATTR)) { - bv.push_back(po.attrs.find(OI_ATTR)->second); + bv = po.attrs.find(OI_ATTR)->second; } auto bliter = bv.cbegin(); decode(oi, bliter); @@ -634,9 +634,8 @@ shard_as_auth_t ScrubBackend::possible_auth_shard(const hobject_t& obj, &shard_info_wrapper::set_snapset_missing, "candidate had a missing snapset key"sv, errstream)) { - bufferlist ss_bl; + const bufferlist& ss_bl = k->second; SnapSet snapset; - ss_bl.push_back(k->second); try { auto bliter = ss_bl.cbegin(); decode(snapset, bliter); @@ -671,9 +670,8 @@ shard_as_auth_t ScrubBackend::possible_auth_shard(const hobject_t& obj, &shard_info_wrapper::set_hinfo_missing, "candidate had a missing hinfo key"sv, errstream)) { - bufferlist hk_bl; + const bufferlist& hk_bl = k->second; ECUtil::HashInfo hi; - hk_bl.push_back(k->second); try { auto bliter = hk_bl.cbegin(); decode(hi, bliter); @@ -704,10 +702,8 @@ shard_as_auth_t ScrubBackend::possible_auth_shard(const hobject_t& obj, return shard_as_auth_t{errstream.str()}; } - bufferlist bl; - bl.push_back(k->second); try { - auto bliter = bl.cbegin(); + auto bliter = k->second.cbegin(); decode(oi, bliter); } catch (...) { // invalid object info, probably corrupt @@ -1232,13 +1228,11 @@ bool ScrubBackend::compare_obj_details(pg_shard_t auth_shard, auto can_attr = candidate.attrs.find(OI_ATTR); ceph_assert(can_attr != candidate.attrs.end()); - bufferlist can_bl; - can_bl.push_back(can_attr->second); + const bufferlist& can_bl = can_attr->second; auto auth_attr = auth.attrs.find(OI_ATTR); ceph_assert(auth_attr != auth.attrs.end()); - bufferlist auth_bl; - auth_bl.push_back(auth_attr->second); + const bufferlist& auth_bl = auth_attr->second; if (!can_bl.contents_equal(auth_bl)) { fmt::format_to(std::back_inserter(out), @@ -1254,13 +1248,11 @@ bool ScrubBackend::compare_obj_details(pg_shard_t auth_shard, auto can_attr = candidate.attrs.find(SS_ATTR); ceph_assert(can_attr != candidate.attrs.end()); - bufferlist can_bl; - can_bl.push_back(can_attr->second); + const bufferlist& can_bl = can_attr->second; auto auth_attr = auth.attrs.find(SS_ATTR); ceph_assert(auth_attr != auth.attrs.end()); - bufferlist auth_bl; - auth_bl.push_back(auth_attr->second); + const bufferlist& auth_bl = auth_attr->second; if (!can_bl.contents_equal(auth_bl)) { fmt::format_to(std::back_inserter(out), @@ -1279,13 +1271,11 @@ bool ScrubBackend::compare_obj_details(pg_shard_t auth_shard, auto can_hi = candidate.attrs.find(ECUtil::get_hinfo_key()); ceph_assert(can_hi != candidate.attrs.end()); - bufferlist can_bl; - can_bl.push_back(can_hi->second); + const bufferlist& can_bl = can_hi->second; auto auth_hi = auth.attrs.find(ECUtil::get_hinfo_key()); ceph_assert(auth_hi != auth.attrs.end()); - bufferlist auth_bl; - auth_bl.push_back(auth_hi->second); + const bufferlist& auth_bl = auth_hi->second; if (!can_bl.contents_equal(auth_bl)) { fmt::format_to(std::back_inserter(out), @@ -1351,7 +1341,7 @@ bool ScrubBackend::compare_obj_details(pg_shard_t auth_shard, sep(error), k); obj_result.set_attr_name_mismatch(); - } else if (cand->second.cmp(v)) { + } else if (!cand->second.contents_equal(v)) { fmt::format_to(std::back_inserter(out), "{}attr value mismatch '{}'", sep(error), @@ -1463,10 +1453,8 @@ void ScrubBackend::scrub_snapshot_metadata(ScrubMap& map) this_chunk->m_error_counts.shallow_errors++; soid_error.set_info_missing(); } else { - bufferlist bv; - bv.push_back(p->second.attrs[OI_ATTR]); try { - oi = object_info_t(bv); + oi = object_info_t(std::as_const(p->second.attrs[OI_ATTR])); } catch (ceph::buffer::error& e) { oi = std::nullopt; clog.error() << m_mode_desc << " " << m_pg_id << " " << soid @@ -1592,13 +1580,11 @@ void ScrubBackend::scrub_snapshot_metadata(ScrubMap& map) snapset = std::nullopt; head_error.set_snapset_missing(); } else { - bufferlist bl; - bl.push_back(p->second.attrs[SS_ATTR]); - auto blp = bl.cbegin(); + auto blp = p->second.attrs[SS_ATTR].cbegin(); try { snapset = SnapSet(); // Initialize optional<> before decoding into it decode(*snapset, blp); - head_error.ss_bl.push_back(p->second.attrs[SS_ATTR]); + head_error.ss_bl.append(p->second.attrs[SS_ATTR]); } catch (ceph::buffer::error& e) { snapset = std::nullopt; clog.error() << m_mode_desc << " " << m_pg_id << " " << soid @@ -1789,13 +1775,11 @@ std::vector ScrubBackend::scan_snaps( if (hoid.is_head()) { // parse the SnapSet - bufferlist bl; if (o.attrs.find(SS_ATTR) == o.attrs.end()) { // no snaps for this head continue; } - bl.push_back(o.attrs[SS_ATTR]); - auto p = bl.cbegin(); + auto p = o.attrs[SS_ATTR].cbegin(); try { decode(snapset, p); } catch (...) { diff --git a/src/test/crimson/test_crimson_scrub.cc b/src/test/crimson/test_crimson_scrub.cc index 65b1f31525390..488d2246436d0 100644 --- a/src/test/crimson/test_crimson_scrub.cc +++ b/src/test/crimson/test_crimson_scrub.cc @@ -24,22 +24,21 @@ constexpr static size_t TEST_OMAP_BYTES_LIMIT = 1<<30; void so_set_attr_len(ScrubMap::object &obj, const std::string &name, size_t len) { - obj.attrs[name] = buffer::ptr(len); + obj.attrs[name] = bufferlist(); + obj.attrs[name].push_back(buffer::ptr(len)); } void so_set_attr(ScrubMap::object &obj, const std::string &name, bufferlist bl) { bl.rebuild(); - obj.attrs[name] = bl.front(); + obj.attrs[name] = bl; } std::optional so_get_attr( ScrubMap::object &obj, const std::string &name) { if (obj.attrs.count(name)) { - bufferlist bl; - bl.push_back(obj.attrs[name]); - return bl; + return obj.attrs[name]; } else { return std::nullopt; } diff --git a/src/test/osd/scrubber_generators.cc b/src/test/osd/scrubber_generators.cc index 0f2f371e714b9..19f64bb05ad87 100644 --- a/src/test/osd/scrubber_generators.cc +++ b/src/test/osd/scrubber_generators.cc @@ -8,7 +8,7 @@ using namespace ScrubGenerator; // ref: PGLogTestRebuildMissing() -bufferptr create_object_info(const ScrubGenerator::RealObj& objver) +bufferlist create_object_info(const ScrubGenerator::RealObj& objver) { object_info_t oi{}; oi.soid = objver.ghobj.hobj; @@ -18,25 +18,23 @@ bufferptr create_object_info(const ScrubGenerator::RealObj& objver) bufferlist bl; oi.encode(bl, 0 /*get_osdmap()->get_features(CEPH_ENTITY_TYPE_OSD, nullptr)*/); - bufferptr bp(bl.c_str(), bl.length()); - return bp; + return bl; } -std::pair> create_object_snapset( +std::pair> create_object_snapset( const ScrubGenerator::RealObj& robj, const SnapsetMockData* snapset_mock_data) { if (!snapset_mock_data) { - return {bufferptr(), {}}; + return {bufferlist(), {}}; } /// \todo fill in missing version/osd details from the robj auto sns = snapset_mock_data->make_snapset(); bufferlist bl; encode(sns, bl); - bufferptr bp = bufferptr(bl.c_str(), bl.length()); // extract the set of object snaps - return {bp, sns.snaps}; + return {bl, sns.snaps}; } RealObjsConfList ScrubGenerator::make_real_objs_conf( @@ -70,9 +68,9 @@ ScrubGenerator::SmapEntry ScrubGenerator::make_smobject( ret.ghobj = blueprint.ghobj; ret.smobj.attrs[OI_ATTR] = create_object_info(blueprint); if (blueprint.snapset_mock_data) { - auto [bp, snaps] = + auto [bl, snaps] = create_object_snapset(blueprint, blueprint.snapset_mock_data); - ret.smobj.attrs[SS_ATTR] = bp; + ret.smobj.attrs[SS_ATTR] = bl; std::cout << fmt::format("{}: ({}) osd:{} snaps:{}", __func__, ret.ghobj.hobj, @@ -82,12 +80,12 @@ ScrubGenerator::SmapEntry ScrubGenerator::make_smobject( } for (const auto& [at_k, at_v] : blueprint.data.attrs) { - ret.smobj.attrs[at_k] = ceph::buffer::copy(at_v.c_str(), at_v.size()); + // deep copy assignment + ret.smobj.attrs[at_k].clear(); + ret.smobj.attrs[at_k].append(at_v.c_str(), at_v.size()); { // verifying (to be removed after dev phase) - auto bk = ret.smobj.attrs[at_k].begin_deep().get_ptr( - ret.smobj.attrs[at_k].length()); - std::string bkstr{bk.raw_c_str(), bk.raw_length()}; + std::string bkstr = ret.smobj.attrs[at_k].to_str(); std::cout << fmt::format("{}: verification: {}", __func__, bkstr) << std::endl; }