]> git.apps.os.sepia.ceph.com Git - ceph.git/commitdiff
osd: ECBackend::get_hash_info() makes use of the obc::attr_cache
authorRadosław Zarzyński <rzarzyns@redhat.com>
Tue, 7 Nov 2023 15:22:46 +0000 (16:22 +0100)
committerRadoslaw Zarzynski <rzarzyns@redhat.com>
Wed, 10 Jan 2024 17:30:28 +0000 (17:30 +0000)
Signed-off-by: Radosław Zarzyński <rzarzyns@redhat.com>
(cherry picked from commit cf07db1a09044c689f5b595d5088a61e3d098e15)

12 files changed:
src/common/scrub_types.cc
src/crimson/osd/osd_operations/scrub_events.cc
src/crimson/osd/scrub/scrub_validator.cc
src/osd/ECBackend.cc
src/osd/ECBackend.h
src/osd/osd_types.cc
src/osd/osd_types.h
src/osd/osd_types_fmt.h
src/osd/scrubber/pg_scrubber.cc
src/osd/scrubber/scrub_backend.cc
src/test/crimson/test_crimson_scrub.cc
src/test/osd/scrubber_generators.cc

index 9168ee0a27939722f421e916b33b31b1d6bf05f5..b03a3cab70c8427a3ee4de8e5233dd0852232a40 100644 (file)
@@ -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) {
index 4f54cf0b274f304af642c881e95689c98105a029..a488077d6a77f0f898cd36d1e6b5a992fd86cb6b 100644 (file)
@@ -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(
index b7dcc46a35e7c3f073e8bbedfa45970df95207a7..a3979f790bca4d1e99b9d1bc43f73e77e69f9a4e 100644 (file)
@@ -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();
index 1322f7f47f315716de7c48315a764c376d1597ff..836bf96dde445308b12c2060e21576e09da96fb3 100644 (file)
@@ -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;
 
index 54b8e16119ebf62b6cc659f1f529dfb365394ea0..27176bf18f927fb919c231c50ff5900f1f2fdb0d 100644 (file)
@@ -325,8 +325,8 @@ public:
   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(
index 7596723a0e30d49abbeb2ac3859fb4026eb23251..1a6b6041d66fe1fdb7820b6332e7cf857522537b 100644 (file)
@@ -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 --
index ff02ca5730f78c80a9940bf9b90459a1f072595c..a8e6b5994a93b9c2eeb1129dbddb8beb45d7eb3c 100644 (file)
@@ -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
index d6d746d295f75de26b22a9030d660a53adc4fae9..6ffe95a3e95fcd00153b26f398a9f28dfbefb4ff 100644 (file)
@@ -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());
index 2dae53273a7059a5b4c338ce0a69c0b7791e550b..3bfc3a10f10a4cadf70ef665812d282bc364f2dd 100644 (file)
@@ -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));
index e25c5b99da09c7eeaad4adca6733cfd36b16cce6..2d7d1a4ecf1748ea70e541488aa0c9bc0f167552 100644 (file)
@@ -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 (...) {
index 65b1f31525390d7a7665dc9aaefe9134226eea96..488d2246436d0d87e4c98ecd822e5467f44ad977 100644 (file)
@@ -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<bufferlist> 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;
   }
index 0f2f371e714b9e87e9ae1d5954f36cc7d4c992d5..19f64bb05ad8728ae51d175d37671499d1cb5804 100644 (file)
@@ -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<bufferptr, std::vector<snapid_t>> create_object_snapset(
+std::pair<bufferlist, std::vector<snapid_t>> 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;
     }