osd: ECBackend::get_hash_info() makes use of the obc::attr_cache

Signed-off-by: Radosław Zarzyński <rzarzyns@redhat.com>
(cherry picked from commit cf07db1)

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<void>
+  // while avoiding `reinterpret_cast<const std::map<std::string,
+  // ceph::bufferlist>&>(object.attrs)`
+  attrs.clear();
+  for (const auto& kv : object.attrs) {
+    attrs.insert(kv);
   }
   size = object.size;
   if (object.omap_digest_present) {

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(

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();

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<string, bufferlist, less<>> 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<string,bufferptr,less<>> *attrs)
+  const hobject_t &hoid, bool create, const map<string,bufferlist,less<>> *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<string, bufferptr>::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<string, bufferlist>::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<string, bufferlist, less<>> *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;
 

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<hobject_t, ECUtil::HashInfo> unstable_hashinfo_registry;
-  ECUtil::HashInfoRef get_hash_info(const hobject_t &hoid, bool create = false,
-				    const std::map<std::string, ceph::buffer::ptr, std::less<>> *attr = NULL);
+  ECUtil::HashInfoRef get_hash_info(const hobject_t &hoid, bool create,
+				    const std::map<std::string, ceph::buffer::list, std::less<>> *attr);
 
 public:
   ECBackend(

src/osd/osd_types.cc

@@ -7148,8 +7148,16 @@ void ScrubMap::object::generate_test_instances(list<object*>& 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 --

src/osd/osd_types.h

@@ -6233,7 +6233,7 @@ std::ostream& operator<<(std::ostream& out, const PushOp &op);
  */
 struct ScrubMap {
   struct object {
-    std::map<std::string, ceph::buffer::ptr, std::less<>> attrs;
+    std::map<std::string, ceph::buffer::list, std::less<>> attrs;
     uint64_t size;
     __u32 omap_digest;         ///< omap crc32c
     __u32 digest;              ///< data crc32c

src/osd/osd_types_fmt.h

@@ -275,15 +275,13 @@ struct fmt::formatter<ScrubMap::object> {
 
     // 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(), "{{{}:<<OI_ATTR>>({})}} ", 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());

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));

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<snap_mapper_fix_t> 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 (...) {