Merge pull request ceph#44941 from ronen-fr/wip-rf-scrubBEv1_2

osd/scrub: limiting scrubber-backend external interfaces

Reviewed-by: Samuel Just <[email protected]>

Author: ronen-fr

src/osd/PG.h

@@ -306,12 +306,12 @@ class PG : public DoutPrefixProvider, public PeeringState::PeeringListener {
     stats.objects_scrubbed = 0;
   }
 
-  void reset_objects_scrubbed() {
-    recovery_state.update_stats(
-      [=](auto &history, auto &stats) {
-  reset_objects_scrubbed(stats);
-  return true;
-      });
+  void reset_objects_scrubbed()
+  {
+    recovery_state.update_stats([=](auto& history, auto& stats) {
+      reset_objects_scrubbed(stats);
+      return true;
+    });
   }
 
   bool is_deleting() const {

src/osd/scrubber/ScrubStore.cc

@@ -124,13 +124,23 @@ Store::~Store()
   ceph_assert(results.empty());
 }
 
+void Store::add_error(int64_t pool, const inconsistent_obj_wrapper& e)
+{
+  add_object_error(pool, e);
+}
+
 void Store::add_object_error(int64_t pool, const inconsistent_obj_wrapper& e)
 {
   bufferlist bl;
   e.encode(bl);
   results[to_object_key(pool, e.object)] = bl;
 }
 
+void Store::add_error(int64_t pool, const inconsistent_snapset_wrapper& e)
+{
+  add_snap_error(pool, e);
+}
+
 void Store::add_snap_error(int64_t pool, const inconsistent_snapset_wrapper& e)
 {
   bufferlist bl;

src/osd/scrubber/ScrubStore.h

@@ -25,6 +25,11 @@ class Store {
 		       const coll_t& coll);
   void add_object_error(int64_t pool, const inconsistent_obj_wrapper& e);
   void add_snap_error(int64_t pool, const inconsistent_snapset_wrapper& e);
+
+  // and a variant-friendly interface:
+  void add_error(int64_t pool, const inconsistent_obj_wrapper& e);
+  void add_error(int64_t pool, const inconsistent_snapset_wrapper& e);
+
   bool empty() const;
   void flush(ObjectStore::Transaction *);
   void cleanup(ObjectStore::Transaction *);

src/osd/scrubber/pg_scrubber.cc

@@ -25,7 +25,6 @@
 
 using std::list;
 using std::pair;
-using std::set;
 using std::stringstream;
 using std::vector;
 using namespace Scrub;
@@ -1039,13 +1038,16 @@ int PgScrubber::build_replica_map_chunk()
     case 0: {
       // finished!
 
-      m_be->replica_clean_meta(replica_scrubmap, m_end.is_max(), m_start);
+      auto required_fixes = m_be->replica_clean_meta(
+        replica_scrubmap, m_end.is_max(), m_start, *this);
+      // actuate snap-mapper changes:
+      apply_snap_mapper_fixes(required_fixes);
 
       // the local map has been created. Send it to the primary.
       // Note: once the message reaches the Primary, it may ask us for another
-      // chunk - and we better be done with the current scrub. Thus - the preparation of
-      // the reply message is separate, and we clear the scrub state before actually
-      // sending it.
+      // chunk - and we better be done with the current scrub. Thus - the
+      // preparation of the reply message is separate, and we clear the scrub
+      // state before actually sending it.
 
       auto reply = prep_replica_map_msg(PreemptionNoted::no_preemption);
       replica_handling_done();
@@ -1129,10 +1131,107 @@ void PgScrubber::run_callbacks()
   }
 }
 
+void PgScrubber::persist_scrub_results(inconsistent_objs_t&& all_errors)
+{
+  dout(10) << __func__ << " " << all_errors.size() << " errors" << dendl;
+
+  for (auto& e : all_errors) {
+    std::visit([this](auto& e) { m_store->add_error(m_pg->pool.id, e); }, e);
+  }
+
+  ObjectStore::Transaction t;
+  m_store->flush(&t);
+  m_osds->store->queue_transaction(m_pg->ch, std::move(t), nullptr);
+}
+
+void PgScrubber::apply_snap_mapper_fixes(
+  const std::vector<snap_mapper_fix_t>& fix_list)
+{
+  dout(15) << __func__ << " " << fix_list.size() << " fixes" << dendl;
+
+  if (fix_list.empty()) {
+    return;
+  }
+
+  ObjectStore::Transaction t;
+  OSDriver::OSTransaction t_drv(m_pg->osdriver.get_transaction(&t));
+
+  for (auto& [fix_op, hoid, snaps, bogus_snaps] : fix_list) {
+
+    if (fix_op == snap_mapper_op_t::update) {
+
+      // must remove the existing snap-set before inserting the correct one
+      if (auto r = m_pg->snap_mapper.remove_oid(hoid, &t_drv); r < 0) {
+
+        derr << __func__ << ": remove_oid returned " << cpp_strerror(r)
+             << dendl;
+        ceph_abort();
+      }
+
+      m_osds->clog->error() << fmt::format(
+        "osd.{} found snap mapper error on pg {} oid {} snaps in mapper: {}, "
+        "oi: "
+        "{} ...repaired",
+        m_pg_whoami, m_pg_id, hoid, bogus_snaps, snaps);
+
+    } else {
+
+      m_osds->clog->error() << fmt::format(
+        "osd.{} found snap mapper error on pg {} oid {} snaps missing in "
+        "mapper, should be: {} ...repaired",
+        m_pg_whoami, m_pg_id, hoid, snaps);
+    }
+
+    // now - insert the correct snap-set
+
+    m_pg->snap_mapper.add_oid(hoid, snaps, &t_drv);
+  }
+
+  // wait for repair to apply to avoid confusing other bits of the system.
+  {
+    dout(15) << __func__ << " wait on repair!" << dendl;
+
+    ceph::condition_variable my_cond;
+    ceph::mutex my_lock = ceph::make_mutex("PG::_scan_snaps my_lock");
+    int e = 0;
+    bool done{false};
+
+    t.register_on_applied_sync(new C_SafeCond(my_lock, my_cond, &done, &e));
+
+    if (e = m_pg->osd->store->queue_transaction(m_pg->ch, std::move(t));
+        e != 0) {
+      derr << __func__ << ": queue_transaction got " << cpp_strerror(e)
+           << dendl;
+    } else {
+      std::unique_lock l{my_lock};
+      my_cond.wait(l, [&done] { return done; });
+      ceph_assert(m_pg->osd->store);  // RRR why?
+    }
+    dout(15) << __func__ << " wait on repair - done" << dendl;
+  }
+}
+
 void PgScrubber::maps_compare_n_cleanup()
 {
   m_pg->add_objects_scrubbed_count(m_be->get_primary_scrubmap().objects.size());
-  m_be->scrub_compare_maps(m_end.is_max());
+
+  auto required_fixes = m_be->scrub_compare_maps(m_end.is_max(), *this);
+  if (!required_fixes.inconsistent_objs.empty()) {
+    if (state_test(PG_STATE_REPAIR)) {
+      dout(10) << __func__ << ": discarding scrub results (repairing)" << dendl;
+    } else {
+      // perform the ordered scrub-store I/O:
+      persist_scrub_results(std::move(required_fixes.inconsistent_objs));
+    }
+  }
+
+  // actuate snap-mapper changes:
+  apply_snap_mapper_fixes(required_fixes.snap_fix_list);
+
+  auto chunk_err_counts = m_be->get_error_counts();
+  m_shallow_errors += chunk_err_counts.shallow_errors;
+  m_deep_errors += chunk_err_counts.deep_errors;
+
   m_start = m_end;
   run_callbacks();
   requeue_waiting();
@@ -1527,20 +1626,23 @@ void PgScrubber::scrub_finish()
   // if the repair request comes from auto-repair and large number of errors,
   // we would like to cancel auto-repair
   if (m_is_repair && m_flags.auto_repair &&
-      m_authoritative.size() > m_pg->cct->_conf->osd_scrub_auto_repair_num_errors) {
+      m_be->authoritative_peers_count() >
+        static_cast<int>(m_pg->cct->_conf->osd_scrub_auto_repair_num_errors)) {
 
     dout(10) << __func__ << " undoing the repair" << dendl;
-    state_clear(PG_STATE_REPAIR); // not expected to be set, anyway
+    state_clear(PG_STATE_REPAIR);  // not expected to be set, anyway
     m_is_repair = false;
     update_op_mode_text();
   }
 
   m_be->update_repair_status(m_is_repair);
 
-  // if a regular scrub had errors within the limit, do a deep scrub to auto repair
+  // if a regular scrub had errors within the limit, do a deep scrub to auto
+  // repair
   bool do_auto_scrub = false;
-  if (m_flags.deep_scrub_on_error && !m_authoritative.empty() &&
-      m_authoritative.size() <= m_pg->cct->_conf->osd_scrub_auto_repair_num_errors) {
+  if (m_flags.deep_scrub_on_error && m_be->authoritative_peers_count() &&
+      m_be->authoritative_peers_count() <=
+        static_cast<int>(m_pg->cct->_conf->osd_scrub_auto_repair_num_errors)) {
     ceph_assert(!m_is_deep);
     do_auto_scrub = true;
     dout(15) << __func__ << " Try to auto repair after scrub errors" << dendl;
@@ -1551,9 +1653,34 @@ void PgScrubber::scrub_finish()
   // type-specific finish (can tally more errors)
   _scrub_finish();
 
+  /// \todo fix the relevant scrub test so that we would not need the extra log
+  /// line here (even if the following 'if' is false)
+
+  if (m_be->authoritative_peers_count()) {
+
+    auto err_msg = fmt::format("{} {} {} missing, {} inconsistent objects",
+                               m_pg->info.pgid,
+                               m_mode_desc,
+                               m_be->m_missing.size(),
+                               m_be->m_inconsistent.size());
+
+    dout(2) << err_msg << dendl;
+    m_osds->clog->error() << fmt::to_string(err_msg);
+  }
+
   // note that the PG_STATE_REPAIR might have changed above
-  m_fixed_count += m_be->scrub_process_inconsistent();
-  bool has_error = !m_authoritative.empty() && m_is_repair;
+  if (m_be->authoritative_peers_count() && m_is_repair) {
+
+    state_clear(PG_STATE_CLEAN);
+    // we know we have a problem, so it's OK to set the user-visible flag
+    // even if we only reached here via auto-repair
+    state_set(PG_STATE_REPAIR);
+    update_op_mode_text();
+    m_be->update_repair_status(true);
+    m_fixed_count += m_be->scrub_process_inconsistent();
+  }
+
+  bool has_error = (m_be->authoritative_peers_count() > 0) && m_is_repair;
 
   {
     stringstream oss;
@@ -2004,7 +2131,6 @@ void PgScrubber::reset_internal_state()
 
   run_callbacks();
 
-  m_authoritative.clear();
   num_digest_updates_pending = 0;
   m_primary_scrubmap_pos.reset();
   replica_scrubmap = ScrubMap{};

src/osd/scrubber/pg_scrubber.h

@@ -255,8 +255,9 @@ ostream& operator<<(ostream& out, const scrub_flags_t& sf);
  * am forced to strongly decouple the state-machine implementation details from
  * the actual scrubbing code.
  */
-class PgScrubber : public ScrubPgIF, public ScrubMachineListener {
-
+class PgScrubber : public ScrubPgIF,
+                   public ScrubMachineListener,
+                   public SnapMapperAccessor {
  public:
   explicit PgScrubber(PG* pg);
 
@@ -509,6 +510,12 @@ class PgScrubber : public ScrubPgIF, public ScrubMachineListener {
   utime_t scrub_begin_stamp;
   std::ostream& gen_prefix(std::ostream& out) const final;
 
+  //  fetching the snap-set for a given object (used by the scrub-backend)
+  int get_snaps(const hobject_t& hoid, std::set<snapid_t>* snaps_set) const final
+  {
+    return m_pg->snap_mapper.get_snaps(hoid, snaps_set);
+  }
+
  protected:
   bool state_test(uint64_t m) const { return m_pg->state_test(m); }
   void state_set(uint64_t m) { m_pg->state_set(m); }
@@ -815,9 +822,8 @@ class PgScrubber : public ScrubPgIF, public ScrubMachineListener {
 
   Scrub::MapsCollectionStatus m_maps_status;
 
-
-  /// Maps from object with errors to good peers
-  std::map<hobject_t, std::list<std::pair<ScrubMap::object, pg_shard_t>>> m_authoritative;
+  void persist_scrub_results(inconsistent_objs_t&& all_errors);
+  void apply_snap_mapper_fixes(const std::vector<snap_mapper_fix_t>& fix_list);
 
   // ------------ members used if we are a replica