osd: Remove all references to hinfo from optimized EC

    Legacy EC used hinfo to store two things:
    1. Shard size
    2. CRCs of the shards

    However:
    * Optimized EC stores different object sizes on each shard
    * Optimized EC scrub calculates the correct sizes of shards and checks them, so shard size checks are not needed in hinfo.
    * Bluestore checks the CRC.
    * Seastore checks the CRC.

    As such, the hinfo object is redundant. As such we remove it in
    optimized EC:

    1. Remove all references/upgrades to hinfo.
    2. Delete hinfo attribute if found on recovery/backfill.
    3. Redirect all scrub references for hinfo to legacy EC.

Signed-off-by: Alex Ainscow <[email protected]>

Author: aainscow

src/crimson/osd/scrub/scrub_validator.cc

@@ -8,6 +8,7 @@
 #include "crimson/common/log.h"
 #include "crimson/osd/scrub/scrub_validator.h"
 #include "osd/ECUtil.h"
+#include "osd/ECUtilL.h"
 
 SET_SUBSYS(osd);
 
@@ -31,7 +32,7 @@ struct shard_evaluation_t {
 
   std::optional<object_info_t> object_info;
   std::optional<SnapSet> snapset;
-  std::optional<ECUtil::HashInfo> hinfo;
+  std::optional<ECLegacy::ECUtilL::HashInfo> hinfo;
 
   size_t omap_keys{0};
   size_t omap_bytes{0};
@@ -132,11 +133,11 @@ shard_evaluation_t evaluate_object_shard(
   }
 
   if (policy.is_ec()) {
-    auto xiter = obj.attrs.find(ECUtil::get_hinfo_key());
+    auto xiter = obj.attrs.find(ECLegacy::ECUtilL::get_hinfo_key());
     if (xiter == obj.attrs.end()) {
       ret.shard_info.set_hinfo_missing();
     } else {
-      ret.hinfo = ECUtil::HashInfo{};
+      ret.hinfo = ECLegacy::ECUtilL::HashInfo{};
       try {
 	auto bliter = xiter->second.cbegin();
 	decode(*(ret.hinfo), bliter);
@@ -210,10 +211,10 @@ librados::obj_err_t compare_candidate_to_authoritative(
   }
 
   if (policy.is_ec()) {
-    auto aiter = auth_si.attrs.find(ECUtil::get_hinfo_key());
+    auto aiter = auth_si.attrs.find(ECLegacy::ECUtilL::get_hinfo_key());
     ceph_assert(aiter != auth_si.attrs.end());
 
-    auto citer = cand_si.attrs.find(ECUtil::get_hinfo_key());
+    auto citer = cand_si.attrs.find(ECLegacy::ECUtilL::get_hinfo_key());
     if (citer == cand_si.attrs.end() ||
 	!aiter->second.contents_equal(citer->second)) {
       ret.errors |= obj_err_t::HINFO_INCONSISTENCY;
@@ -226,7 +227,7 @@ librados::obj_err_t compare_candidate_to_authoritative(
 
   auto is_sys_attr = [&policy](const auto &str) {
     return str == OI_ATTR || str == SS_ATTR ||
-      (policy.is_ec() && str == ECUtil::get_hinfo_key());
+      (policy.is_ec() && str == ECLegacy::ECUtilL::get_hinfo_key());
   };
   for (auto aiter = auth_si.attrs.begin(); aiter != auth_si.attrs.end(); ++aiter) {
     if (is_sys_attr(aiter->first)) continue;

src/erasure-code/consistency/ECEncoder.cc

@@ -106,7 +106,7 @@ std::optional<ceph::bufferlist> ECEncoder<stripe_info_o_t>::do_encode(ceph::buff
 
   sinfo.ro_range_to_shard_extent_map(0, inbl.length(), inbl, encoded_data);
   encoded_data.insert_parity_buffers();
-  int r = encoded_data.encode(ec_impl, nullptr, encoded_data.get_ro_end());
+  int r = encoded_data.encode(ec_impl);
   if (r < 0) {
     std::cerr << "Failed to encode: " << cpp_strerror(r) << std::endl;
     return {};

src/osd/ECBackend.cc

@@ -90,10 +90,9 @@ ECBackend::ECBackend(
     rmw_pipeline(cct, ec_impl, this->sinfo, get_parent()->get_eclistener(),
                  *this, ec_extent_cache_lru),
     recovery_backend(cct, switcher->coll, ec_impl, this->sinfo, read_pipeline,
-                     unstable_hashinfo_registry, get_parent(), this),
+                      get_parent(), this),
     ec_impl(ec_impl),
-    sinfo(ec_impl, &(get_parent()->get_pool()), stripe_width),
-    unstable_hashinfo_registry(cct, ec_impl) {
+    sinfo(ec_impl, &(get_parent()->get_pool()), stripe_width) {
 
   /* EC makes some assumptions about how the plugin organises the *data* shards:
    * - The chunk size is constant for a particular profile.
@@ -113,15 +112,13 @@ ECBackend::RecoveryBackend::RecoveryBackend(
   ceph::ErasureCodeInterfaceRef ec_impl,
   const ECUtil::stripe_info_t &sinfo,
   ReadPipeline &read_pipeline,
-  UnstableHashInfoRegistry &unstable_hashinfo_registry,
   ECListener *parent,
   ECBackend *ecbackend)
   : cct(cct),
     coll(coll),
     ec_impl(std::move(ec_impl)),
     sinfo(sinfo),
     read_pipeline(read_pipeline),
-    unstable_hashinfo_registry(unstable_hashinfo_registry),
     parent(parent),
     ecbackend(ecbackend) {}
 
@@ -335,17 +332,6 @@ void ECBackend::RecoveryBackend::handle_recovery_read_complete(
       op.recovery_info.size = op.obc->obs.oi.size;
       op.recovery_info.oi = op.obc->obs.oi;
     }
-
-    if (sinfo.get_is_hinfo_required()) {
-      ECUtil::HashInfo hinfo(sinfo.get_k_plus_m());
-      if (op.obc->obs.oi.size > 0) {
-        ceph_assert(op.xattrs.count(ECUtil::get_hinfo_key()));
-        auto bp = op.xattrs[ECUtil::get_hinfo_key()].cbegin();
-        decode(hinfo, bp);
-      }
-      op.hinfo = unstable_hashinfo_registry.maybe_put_hash_info(
-        hoid, std::move(hinfo));
-    }
   }
   ceph_assert(op.xattrs.size());
   ceph_assert(op.obc);
@@ -561,29 +547,6 @@ void ECBackend::RecoveryBackend::continue_recovery_op(
 
       if (op.recovery_progress.first && op.obc) {
         op.xattrs = op.obc->attr_cache;
-        if (sinfo.get_is_hinfo_required()) {
-          if (auto [r, attrs, size] = ecbackend->get_attrs_n_size_from_disk(
-              op.hoid);
-            r >= 0 || r == -ENOENT) {
-            op.hinfo = unstable_hashinfo_registry.get_hash_info(
-              op.hoid, false, attrs, size);
-          } else {
-            derr << __func__ << ": can't stat-or-getattr on " << op.hoid <<
-          dendl;
-          }
-          if (!op.hinfo) {
-            derr << __func__ << ": " << op.hoid << " has inconsistent hinfo"
-               << dendl;
-            ceph_assert(recovery_ops.count(op.hoid));
-            eversion_t v = recovery_ops[op.hoid].v;
-            recovery_ops.erase(op.hoid);
-            // TODO: not in crimson yet
-            get_parent()->on_failed_pull({get_parent()->whoami_shard()},
-                                         op.hoid, v);
-            return;
-          }
-          encode(*(op.hinfo), op.xattrs[ECUtil::get_hinfo_key()]);
-        }
       }
 
       read_request_t read_request(std::move(want),
@@ -675,6 +638,12 @@ void ECBackend::RecoveryBackend::continue_recovery_op(
             dout(10) << __func__ << ": push all attrs (not nonprimary)" << dendl;
             pop.attrset = op.xattrs;
           }
+
+          // Following an upgrade, or turning of overwrites, we can take this
+          // opportunity to clean up hinfo.
+          if (pop.attrset.contains(ECUtil::get_hinfo_key())) {
+            pop.attrset.erase(ECUtil::get_hinfo_key());
+          }
         }
         pop.recovery_info = op.recovery_info;
         pop.before_progress = op.recovery_progress;
@@ -1100,53 +1069,6 @@ void ECBackend::handle_sub_read(
           << bl.length() << dendl;
         reply->buffers_read[hoid].push_back(make_pair(offset, bl));
       }
-
-      if (!sinfo.supports_ec_overwrites()) {
-        // This shows that we still need deep scrub because large enough files
-        // are read in sections, so the digest check here won't be done here.
-        // 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;
-        map<string, bufferlist, less<>> attrs;
-        struct stat st;
-        int r = object_stat(hoid, &st);
-        if (r >= 0) {
-          dout(10) << __func__ << ": found on disk, size " << st.st_size << dendl;
-          r = switcher->objects_get_attrs_with_hinfo(hoid, &attrs);
-        }
-        if (r >= 0) {
-          hinfo = unstable_hashinfo_registry.get_hash_info(
-            hoid, false, attrs, st.st_size);
-        } else {
-          derr << __func__ << ": access (attrs) on " << hoid << " failed: "
-	       << cpp_strerror(r) << dendl;
-        }
-        if (!hinfo) {
-          r = -EIO;
-          get_parent()->clog_error() << "Corruption detected: object "
-            << hoid << " is missing hash_info";
-          dout(5) << __func__ << ": No hinfo for " << hoid << dendl;
-          goto error;
-        }
-        ceph_assert(hinfo->has_chunk_hash());
-        if ((bl.length() == hinfo->get_total_chunk_size()) &&
-          (offset == 0)) {
-          dout(20) << __func__ << ": Checking hash of " << hoid << dendl;
-          bufferhash h(-1);
-          h << bl;
-          if (h.digest() != hinfo->get_chunk_hash(shard)) {
-            get_parent()->clog_error() << "Bad hash for " << hoid <<
-              " digest 0x"
-              << hex << h.digest() << " expected 0x" << hinfo->
-              get_chunk_hash(shard) << dec;
-            dout(5) << __func__ << ": Bad hash for " << hoid << " digest 0x"
-		    << hex << h.digest() << " expected 0x"
-                    << hinfo->get_chunk_hash(shard) << dec << dendl;
-            r = -EIO;
-            goto error;
-          }
-        }
-      }
     }
     continue;
   error:
@@ -1229,9 +1151,8 @@ void ECBackend::handle_sub_read_reply(
          ++i) {
       if (ECInject::test_read_error0(
         ghobject_t(i->first, ghobject_t::NO_GEN, op.from.shard))) {
-        dout(0) << __func__ << " Error inject - EIO error for shard " << op.from
-                                                                           .shard
- << dendl;
+        dout(0) << __func__ << " Error inject - EIO error for shard "
+                << op.from.shard << dendl;
         op.buffers_read.erase(i->first);
         op.attrs_read.erase(i->first);
         op.errors[i->first] = -EIO;
@@ -1513,14 +1434,6 @@ std::tuple<
   return {0, real_attrs, st.st_size};
 }
 
-ECUtil::HashInfoRef ECBackend::get_hinfo_from_disk(hobject_t oid) {
-  auto [r, attrs, size] = get_attrs_n_size_from_disk(oid);
-  ceph_assert(r >= 0 || r == -ENOENT);
-  ECUtil::HashInfoRef hinfo = unstable_hashinfo_registry.get_hash_info(
-    oid, true, attrs, size);
-  return hinfo;
-}
-
 std::optional<object_info_t> ECBackend::get_object_info_from_obc(
     ObjectContextRef &obc) {
   std::optional<object_info_t> ret;
@@ -1578,21 +1491,12 @@ void ECBackend::submit_transaction(
   op->t->safe_create_traverse(
     [&](std::pair<const hobject_t, PGTransaction::ObjectOperation> &i) {
       const auto &[oid, inner_op] = i;
-      ECUtil::HashInfoRef shinfo;
       auto &obc = obc_map.at(oid);
       object_info_t oi = obc->obs.oi;
       std::optional<object_info_t> soi;
-      ECUtil::HashInfoRef hinfo;
-
-      if (!sinfo.supports_ec_overwrites()) {
-        hinfo = get_hinfo_from_disk(oid);
-      }
 
       hobject_t source;
       if (inner_op.has_source(&source)) {
-        if (!sinfo.supports_ec_overwrites()) {
-          shinfo = get_hinfo_from_disk(source);
-        }
         if (!inner_op.is_rename()) {
           soi = get_object_info_from_obc(obc_map.at(source));
         }
@@ -1619,8 +1523,7 @@ void ECBackend::submit_transaction(
       ECTransaction::WritePlanObj plan(oid, inner_op, sinfo, readable_shards,
                                        writable_shards,
                                        object_in_cache, old_object_size,
-                                       oi, soi, std::move(hinfo),
-                                       std::move(shinfo),
+                                       oi, soi,
                                        rmw_pipeline.ec_pdw_write_mode);
 
       if (plan.to_read) plans.want_read = true;
@@ -1857,53 +1760,7 @@ int ECBackend::be_deep_scrub(
     return -EINPROGRESS;
   }
 
-  if (!sinfo.get_is_hinfo_required()) {
-    o.digest = 0;
-    o.digest_present = true;
-    o.omap_digest = -1;
-    o.omap_digest_present = true;
-    return 0;
-  }
-
-  ECUtil::HashInfoRef hinfo = unstable_hashinfo_registry.get_hash_info(
-    poid, false, o.attrs, o.size);
-  if (!hinfo) {
-    dout(0) << "_scan_list  " << poid << " could not retrieve hash info" << dendl;
-    o.read_error = true;
-    o.digest_present = false;
-    return 0;
-  }
-  if (!hinfo->has_chunk_hash()) {
-    dout(0) << "_scan_list  " << poid << " got invalid hash info" << dendl;
-    o.ec_size_mismatch = true;
-    return 0;
-  }
-  if (hinfo->get_total_chunk_size() != (unsigned)pos.data_pos) {
-    dout(0) << "_scan_list  " << poid << " got incorrect size on read 0x"
-	    << std::hex << pos
-	    << " expected 0x" << hinfo->get_total_chunk_size() << std::dec
-	    << dendl;
-    o.ec_size_mismatch = true;
-    return 0;
-  }
-
-  if (hinfo->get_chunk_hash(get_parent()->whoami_shard().shard) !=
-    pos.data_hash.digest()) {
-    dout(0) << "_scan_list  " << poid << " got incorrect hash on read 0x"
-	    << std::hex << pos.data_hash.digest() << " !=  expected 0x"
-	    << hinfo->get_chunk_hash(get_parent()->whoami_shard().shard)
-	    << std::dec << dendl;
-    o.ec_hash_mismatch = true;
-    return 0;
-  }
-
-  /* We checked above that we match our own stored hash.  We cannot
-   * send a hash of the actual object, so instead we simply send
-   * our locally stored hash of shard 0 on the assumption that if
-   * we match our chunk hash and our recollection of the hash for
-   * chunk 0 matches that of our peers, there is likely no corruption.
-   */
-  o.digest = hinfo->get_chunk_hash(shard_id_t(0));
+  o.digest = 0;
   o.digest_present = true;
   o.omap_digest = -1;
   o.omap_digest_present = true;

src/osd/ECBackend.h

@@ -217,7 +217,6 @@ class ECBackend : public ECCommon {
     ceph::ErasureCodeInterfaceRef ec_impl;
     const ECUtil::stripe_info_t &sinfo;
     ReadPipeline &read_pipeline;
-    UnstableHashInfoRegistry &unstable_hashinfo_registry;
     // TODO: lay an interface down here
     ECListener *parent;
     ECBackend *ecbackend;
@@ -244,7 +243,6 @@ class ECBackend : public ECCommon {
                     ceph::ErasureCodeInterfaceRef ec_impl,
                     const ECUtil::stripe_info_t &sinfo,
                     ReadPipeline &read_pipeline,
-                    UnstableHashInfoRegistry &unstable_hashinfo_registry,
                     ECListener *parent,
                     ECBackend *ecbackend);
 
@@ -278,7 +276,6 @@ class ECBackend : public ECCommon {
       // must be filled if state == WRITING
       std::optional<ECUtil::shard_extent_map_t> returned_data;
       std::map<std::string, ceph::buffer::list, std::less<>> xattrs;
-      ECUtil::HashInfoRef hinfo;
       ObjectContextRef obc;
       std::set<pg_shard_t> waiting_on_pushes;
 
@@ -352,12 +349,10 @@ class ECBackend : public ECCommon {
                       ceph::ErasureCodeInterfaceRef ec_impl,
                       const ECUtil::stripe_info_t &sinfo,
                       ReadPipeline &read_pipeline,
-                      UnstableHashInfoRegistry &unstable_hashinfo_registry,
                       PGBackend::Listener *parent,
                       ECBackend *ecbackend)
       : RecoveryBackend(cct, coll, std::move(ec_impl), sinfo, read_pipeline,
-                        unstable_hashinfo_registry, parent->get_eclistener(),
-                        ecbackend),
+                        parent->get_eclistener(), ecbackend),
         parent(parent) {}
 
     void commit_txn_send_replies(
@@ -481,17 +476,12 @@ class ECBackend : public ECCommon {
 
   const ECUtil::stripe_info_t sinfo;
 
-  ECCommon::UnstableHashInfoRegistry unstable_hashinfo_registry;
-
-
   std::tuple<
     int,
     std::map<std::string, ceph::bufferlist, std::less<>>,
     size_t
   > get_attrs_n_size_from_disk(const hobject_t &hoid);
 
-  ECUtil::HashInfoRef get_hinfo_from_disk(hobject_t oid);
-
   std::optional<object_info_t> get_object_info_from_obc(
       ObjectContextRef &obc_map
     );