From: Sage Weil <sage@redhat.com>
Date: Wed, 11 Sep 2019 17:52:57 +0000 (-0500)
Subject: os/bluestore: simplify per-pool-stat config options
X-Git-Tag: v14.2.5~17^2~1
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=810df58e91f5a96c8c4e06256bad17ee7944600f;p=ceph.git

os/bluestore: simplify per-pool-stat config options

The previous bluestore_no_per_pool_stats_tolerance had a lot of possible
values, not all of which make sense to users.  Replace with a single new
option, bluestore_fsck_error_on_no_per_pool_stats, which controls whether
the lack of per-pool stats is an error or a warning.  On repair, we will
unconditionally convert to per-pool stats.

This brings us in sync with the newer
bluestore_fsck_error_on_no_per_pool_omap.

Note that one part of the ceph_test_objectstore test is dropped since it
is no longer possible to create a store with legacy stats.

Signed-off-by: Sage Weil <sage@redhat.com>
(cherry picked from commit 7e96024254d98591a5431623fb355a20f322b554)

 Conflicts:
	PendingReleaseNotes
        - trivial
	src/os/bluestore/BlueStore.cc
        - lacking of int_repaired method
---

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index 327df79a18b4..24eb68920d6e 100644
--- a/PendingReleaseNotes
+++ b/PendingReleaseNotes
@@ -17,6 +17,12 @@
   multiply your current "objecter_inflight_ops" and
   "objecter_inflight_op_bytes" paramaeters by the old
   "num_rados_handles" to get the same throttle behavior.
+  
+* The ``bluestore_no_per_pool_stats_tolerance`` config option has been
+  replaced with ``bluestore_fsck_error_on_no_per_pool_stats``
+  (default: false).  The overall default behavior has not changed:
+  fsck will warn but not fail on legacy stores, and repair will
+  convert to per-pool stats.
 
 14.2.2
 ------
diff --git a/src/common/legacy_config_opts.h b/src/common/legacy_config_opts.h
index 7e9799567c63..b13dbcb7df6c 100644
--- a/src/common/legacy_config_opts.h
+++ b/src/common/legacy_config_opts.h
@@ -1080,7 +1080,7 @@ OPTION(bluestore_debug_permit_any_bdev_label, OPT_BOOL)
 OPTION(bluestore_debug_random_read_err, OPT_DOUBLE)
 OPTION(bluestore_debug_inject_bug21040, OPT_BOOL)
 OPTION(bluestore_debug_inject_csum_err_probability, OPT_FLOAT)
-OPTION(bluestore_no_per_pool_stats_tolerance, OPT_STR)
+OPTION(bluestore_fsck_error_on_no_per_pool_stats, OPT_BOOL)
 OPTION(bluestore_warn_on_bluefs_spillover, OPT_BOOL)
 OPTION(bluestore_warn_on_legacy_statfs, OPT_BOOL)
 OPTION(bluestore_log_op_age, OPT_DOUBLE)
diff --git a/src/common/options.cc b/src/common/options.cc
index ce055b72b603..07c6e57f7bd4 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -4894,15 +4894,9 @@ std::vector<Option> get_global_options() {
     .set_default(0.0)
     .set_description("inject crc verification errors into bluestore device reads"),
 
-    Option("bluestore_no_per_pool_stats_tolerance", Option::TYPE_STR, Option::LEVEL_ADVANCED)
-    .set_default("until_repair")
-    .set_flag(Option::FLAG_RUNTIME)
-    .set_enum_allowed({"enforce", "until_repair", "until_fsck"})
-    .set_description("Specified how to treat lack of per-pool stats, e.g. caused by an upgrade")
-    .set_long_description(
-      "'until_fsck' will tolerate the case for regular ops and fail on fsck or repair, the latter will fix the issue, "
-      "'until_repair' will tolerate for regular ops and fsck. Repair indicates and fixes the issue, "
-      "'enforce' will unconditionally use global stats mode."),
+    Option("bluestore_fsck_error_on_no_per_pool_stats", Option::TYPE_BOOL, Option::LEVEL_ADVANCED)
+    .set_default(false)
+    .set_description("Make fsck error (instead of warn) when bluestore lacks per-pool stats, e.g., after an upgrade"),
 
     Option("bluestore_warn_on_bluefs_spillover", Option::TYPE_BOOL, Option::LEVEL_ADVANCED)
     .set_default(true)
diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index cf5c034ae44d..6c1ebdcb5b5b 100644
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -3945,7 +3945,6 @@ const char **BlueStore::get_tracked_conf_keys() const
     "osd_memory_cache_min",
     "bluestore_cache_autotune",
     "bluestore_cache_autotune_interval",
-    "bluestore_no_per_pool_stats_tolerance",
     "bluestore_warn_on_legacy_statfs",
     NULL
   };
@@ -3955,8 +3954,7 @@ const char **BlueStore::get_tracked_conf_keys() const
 void BlueStore::handle_conf_change(const ConfigProxy& conf,
 				   const std::set<std::string> &changed)
 {
-  if (changed.count("bluestore_no_per_pool_stats_tolerance") ||
-      changed.count("bluestore_warn_on_legacy_statfs")) {
+  if (changed.count("bluestore_warn_on_legacy_statfs")) {
     _check_legacy_statfs_alert();
   }
 
@@ -5745,9 +5743,6 @@ void BlueStore::_open_statfs()
       dout(10) << __func__ << " store_statfs is corrupt, using empty" << dendl;
     }
     _check_legacy_statfs_alert();
-  } else if (cct->_conf->bluestore_no_per_pool_stats_tolerance == "enforce") {
-    per_pool_stat_collection = false;
-    dout(10) << __func__ << " store_statfs is requested but missing, using empty" << dendl;
   } else {
     per_pool_stat_collection = true;
     dout(10) << __func__ << " per-pool statfs is enabled" << dendl;
@@ -6757,7 +6752,6 @@ int BlueStore::_fsck_check_extents(
 
 void BlueStore::_fsck_check_pool_statfs(
   BlueStore::per_pool_statfs& expected_pool_statfs,
-  bool need_per_pool_stats,
   int64_t& errors,
   int64_t& warnings,
   BlueStoreRepairer* repairer)
@@ -6768,32 +6762,13 @@ void BlueStore::_fsck_check_pool_statfs(
       string key = it->key();
       if (key == BLUESTORE_GLOBAL_STATFS_KEY) {
         if (repairer) {
-	  if (need_per_pool_stats) {
-	    ++errors;
-	    repairer->remove_key(db, PREFIX_STAT, BLUESTORE_GLOBAL_STATFS_KEY);
-	    derr << "fsck error: " << "legacy statfs record found, removing" << dendl;
-	  } else {
-	    derr << "fsck warning: " << "legacy statfs record found, bypassing" << dendl;
-            ++warnings;
-	  }
-	} else {
-	  const char* s;
-          if (need_per_pool_stats) {
-	    ++errors;
-	    s = "fsck error: ";
-	  } else {
-            s = "fsck warning: ";
-            ++warnings;
-          }
-	  derr << s << "legacy statfs record found, suggest to "
-	          "run store repair/quick_fix to get consistent statistic reports"
+	  ++errors;
+	  repairer->remove_key(db, PREFIX_STAT, BLUESTORE_GLOBAL_STATFS_KEY);
+	  derr << "fsck error: " << "legacy statfs record found, removing"
 	       << dendl;
 	}
 	continue;
       }
-      if (!need_per_pool_stats) {
-	continue;
-      }
       uint64_t pool_id;
       if (get_key_pool_stat(key, &pool_id) < 0) {
 	derr << "fsck error: bad key " << key
@@ -6860,26 +6835,31 @@ void BlueStore::_fsck_check_pool_statfs(
       expected_pool_statfs.erase(stat_it);
     }
   } // if (it)
-  for( auto s = expected_pool_statfs.begin(); s != expected_pool_statfs.end();
-    ++s) {
-    if (s->second.is_zero()) {
+  for (auto& s : expected_pool_statfs) {
+    if (s.second.is_zero()) {
       // we might lack empty statfs recs in DB
       continue;
     }
     derr << "fsck error: missing Pool StatFS record for pool "
-	  << std::hex << s->first << std::dec << dendl;
+	 << std::hex << s.first << std::dec << dendl;
     if (repairer) {
       string key;
-      get_pool_stat_key(s->first, &key);
-      repairer->fix_statfs(db, key, s->second);
+      get_pool_stat_key(s.first, &key);
+      repairer->fix_statfs(db, key, s.second);
     }
     ++errors;
   }
+  if (!per_pool_stat_collection &&
+      cct->_conf->bluestore_fsck_error_on_no_per_pool_stats &&
+      repairer) {
+    // by virtue of running this method, we correct the top-level
+    // error of having global stats
+    repairer->inc_repaired();
+  }
 }
 
 BlueStore::OnodeRef BlueStore::fsck_check_objects_shallow(
   BlueStore::FSCKDepth depth,
-  bool need_per_pool_stats,
   int64_t pool_id,
   BlueStore::CollectionRef c,
   const ghobject_t& oid,
@@ -6900,7 +6880,7 @@ BlueStore::OnodeRef BlueStore::fsck_check_objects_shallow(
   auto& sb_info = ctx.sb_info;
   auto repairer = ctx.repairer;
 
-  store_statfs_t* res_statfs = need_per_pool_stats ?
+  store_statfs_t* res_statfs = (per_pool_stat_collection || repairer) ?
     &ctx.expected_pool_statfs[pool_id] :
     &ctx.expected_store_statfs;
 
@@ -7112,7 +7092,6 @@ public:
 
     size_t batchCount;
     BlueStore* store = nullptr;
-    bool need_per_pool_stats;
 
     mempool::bluestore_fsck::list<string>* expecting_shards = nullptr;
     ceph::mutex* sb_info_lock = nullptr;
@@ -7126,7 +7105,6 @@ public:
     FSCKWorkQueue(std::string n,
                   size_t _batchCount,
                   BlueStore* _store,
-                  bool _need_per_pool_stats,
                   mempool::bluestore_fsck::list<string>& _expecting_shards,
                   ceph::mutex* _sb_info_lock,
                   BlueStore::sb_info_map_t& _sb_info,
@@ -7134,7 +7112,6 @@ public:
       WorkQueue_(n, time_t(), time_t()),
       batchCount(_batchCount),
       store(_store),
-      need_per_pool_stats(_need_per_pool_stats),
       expecting_shards(&_expecting_shards),
       sb_info_lock(_sb_info_lock),
       sb_info(&_sb_info),
@@ -7201,7 +7178,6 @@ public:
 
         store->fsck_check_objects_shallow(
           BlueStore::FSCK_SHALLOW,
-          need_per_pool_stats,
           entry.pool_id,
           entry.c,
           entry.oid,
@@ -7314,7 +7290,6 @@ public:
 };
 
 void BlueStore::_fsck_check_objects(FSCKDepth depth,
-  bool need_per_pool_stats,
   BlueStore::FSCK_ObjectCtx& ctx)
 {
   //no need for the below lock when in non-shallow mode as
@@ -7344,7 +7319,6 @@ void BlueStore::_fsck_check_objects(FSCKDepth depth,
         "FSCKWorkQueue",
         (thread_count ? : 1) * 32,
         this,
-        need_per_pool_stats,
         expecting_shards,
         sb_info_lock,
         sb_info,
@@ -7467,7 +7441,6 @@ void BlueStore::_fsck_check_objects(FSCKDepth depth,
 
          o = fsck_check_objects_shallow(
           depth,
-          need_per_pool_stats,
           pool_id,
           c,
           oid,
@@ -7726,10 +7699,6 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair)
   BlueStoreRepairer repairer;
 
   utime_t start = ceph_clock_now();
-  const auto& no_pps_mode = cct->_conf->bluestore_no_per_pool_stats_tolerance;
-  bool need_per_pool_stats = no_pps_mode == "until_fsck" ||
-    ((no_pps_mode == "until_repair")  && repair);
-  bool enforce_no_per_pool_stats = no_pps_mode == "enforce";
 
   _fsck_collections(&errors);
   used_blocks.resize(fm->get_alloc_units());
@@ -7790,6 +7759,18 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair)
       errors += r;
   }
 
+  if (!per_pool_stat_collection) {
+    const char *w;
+    if (cct->_conf->bluestore_fsck_error_on_no_per_pool_stats) {
+      w = "error";
+      ++errors;
+    } else {
+      w = "warning";
+      ++warnings;
+    }
+    derr << "fsck " << w << ": store not yet converted to per-pool stats"
+	 << dendl;
+  }
   // get expected statfs; reset unaffected fields to be able to compare
   // structs
   statfs(&actual_statfs);
@@ -7799,8 +7780,6 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair)
   actual_statfs.internal_metadata = 0;
   actual_statfs.omap_allocated = 0;
 
-  need_per_pool_stats = per_pool_stat_collection || need_per_pool_stats;
-
   if (g_conf()->bluestore_debug_fsck_abort) {
     dout(1) << __func__ << " debug abort" << dendl;
     goto out_scan;
@@ -7827,15 +7806,14 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair)
       expected_pool_statfs,
       repair ? &repairer : nullptr);
     _fsck_check_objects(depth,
-      need_per_pool_stats,
       ctx);
   }
 
   dout(1) << __func__ << " checking shared_blobs" << dendl;
   it = db->get_iterator(PREFIX_SHARED_BLOB);
   if (it) {
-    //FIXME minor: perhaps simplify for shallow mode?
-    //fill global if not overriden below
+    // FIXME minor: perhaps simplify for shallow mode?
+    // fill global if not overriden below
     auto expected_statfs = &expected_store_statfs;
 
     for (it->lower_bound(string()); it->valid(); it->next()) {
@@ -7893,7 +7871,7 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair)
 	for (auto &r : shared_blob.ref_map.ref_map) {
 	  extents.emplace_back(bluestore_pextent_t(r.first, r.second.length));
 	}
-	if (need_per_pool_stats) {
+	if (per_pool_stat_collection || repair) {
 	  expected_statfs = &expected_pool_statfs[sbi.pool_id];
 	}
 	errors += _fsck_check_extents(sbi.cid,
@@ -7918,7 +7896,7 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair)
     interval_set<uint64_t> to_release;
     it = db->get_iterator(PREFIX_OBJ);
     if (it) {
-      //fill global if not overriden below
+      // fill global if not overriden below
       auto expected_statfs = &expected_store_statfs;
 
       CollectionRef c;
@@ -7953,8 +7931,8 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair)
 	  if (!c) {
 	    continue;
 	  }
-	  auto pool_id = c->cid.is_pg(&pgid) ? pgid.pool() : META_POOL_ID;
-	  if (need_per_pool_stats) {
+	  if (per_pool_stat_collection || repair) {
+	    auto pool_id = c->cid.is_pg(&pgid) ? pgid.pool() : META_POOL_ID;
 	    expected_statfs = &expected_pool_statfs[pool_id];
 	  }
 	}
@@ -8134,23 +8112,22 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair)
   }
   sb_info.clear();
 
-  // check global stats if no-pps is enforced only
-  if (!need_per_pool_stats) {
-    if (!(actual_statfs == expected_store_statfs)) {
-      derr << "fsck error: actual " << actual_statfs
-	   << " != expected " << expected_store_statfs << dendl;
-      if (repair) {
-	repairer.fix_statfs(db, BLUESTORE_GLOBAL_STATFS_KEY,
-	  expected_store_statfs);
-      }
-      ++errors;
+  // check global stats only if fscking (not repairing) w/o per-pool stats
+  if (!per_pool_stat_collection &&
+      !repair &&
+      !(actual_statfs == expected_store_statfs)) {
+    derr << "fsck error: actual " << actual_statfs
+	 << " != expected " << expected_store_statfs << dendl;
+    if (repair) {
+      repairer.fix_statfs(db, BLUESTORE_GLOBAL_STATFS_KEY,
+			  expected_store_statfs);
     }
+    ++errors;
   }
-  if (!enforce_no_per_pool_stats) {
-    dout(1) << __func__ << " checking pool_statfs" << dendl;
-    _fsck_check_pool_statfs(expected_pool_statfs, need_per_pool_stats,
-      errors, warnings, repair ? &repairer : nullptr);
-  }
+
+  dout(1) << __func__ << " checking pool_statfs" << dendl;
+  _fsck_check_pool_statfs(expected_pool_statfs,
+			  errors, warnings, repair ? &repairer : nullptr);
 
   if (depth != FSCK_SHALLOW) {
     dout(1) << __func__ << " checking for stray omap data" << dendl;
@@ -8646,8 +8623,7 @@ void BlueStore::_check_legacy_statfs_alert()
 {
   string s;
   if (!per_pool_stat_collection &&
-    cct->_conf->bluestore_no_per_pool_stats_tolerance != "enforce" &&
-    cct->_conf->bluestore_warn_on_legacy_statfs) {
+      cct->_conf->bluestore_warn_on_legacy_statfs) {
     s = "legacy statfs reporting detected, "
         "suggest to run store repair to get consistent statistic reports";
   }
diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index d21ae0dc05af..ec82af35c1b5 100644
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -2354,7 +2354,6 @@ private:
 
   void _fsck_check_pool_statfs(
     per_pool_statfs& expected_pool_statfs,
-    bool need_per_pool_stats,
     int64_t& errors,
     int64_t &warnings,
     BlueStoreRepairer* repairer);
@@ -3140,7 +3139,6 @@ public:
 
   OnodeRef fsck_check_objects_shallow(
     FSCKDepth depth,
-    bool need_per_pool_stats,
     int64_t pool_id,
     CollectionRef c,
     const ghobject_t& oid,
@@ -3152,7 +3150,6 @@ public:
 
 private:
   void _fsck_check_objects(FSCKDepth depth,
-    bool need_per_pool_stats,
     FSCK_ObjectCtx& ctx);
 };
 
@@ -3341,6 +3338,9 @@ public:
       ++to_repair_cnt;
     }
   }
+  void inc_repaired() {
+    ++to_repair_cnt;
+  }  
 
   StoreSpaceTracker& get_space_usage_tracker() {
     return space_usage_tracker;
diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc
index 88d8cc28920a..26b0d33348c4 100644
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -7323,7 +7323,7 @@ TEST_P(StoreTestSpecificAUSize, BluestoreRepairTest) {
   SetVal(g_conf(), "bluestore_max_blob_size", 
     stringify(2 * offs_base).c_str());
   SetVal(g_conf(), "bluestore_extent_map_shard_max_size", "12000");
-  SetVal(g_conf(), "bluestore_no_per_pool_stats_tolerance", "enforce");
+  SetVal(g_conf(), "bluestore_fsck_error_on_no_per_pool_stats", "false");
 
   StartDeferred(0x10000);
 
@@ -7397,7 +7397,7 @@ TEST_P(StoreTestSpecificAUSize, BluestoreRepairTest) {
   bstore->inject_statfs("bluestore_statfs", statfs);
   bstore->umount();
 
-  ASSERT_EQ(bstore->fsck(false), 1);
+  ASSERT_EQ(bstore->fsck(false), 2);
   ASSERT_EQ(bstore->repair(false), 0);
   ASSERT_EQ(bstore->fsck(false), 0);
   ASSERT_EQ(bstore->mount(), 0);
@@ -7463,15 +7463,6 @@ TEST_P(StoreTestSpecificAUSize, BluestoreRepairTest) {
     ASSERT_EQ(bstore->fsck(false), 0);
   }
 
-  // enable per-pool stats collection hence causing fsck to fail
-  cerr << "per-pool statfs" << std::endl;
-  SetVal(g_conf(), "bluestore_no_per_pool_stats_tolerance", "until_fsck");
-  g_ceph_context->_conf.apply_changes(nullptr);
-
-  ASSERT_EQ(bstore->fsck(false), 2);
-  ASSERT_EQ(bstore->repair(false), 0);
-  ASSERT_EQ(bstore->fsck(false), 0);
-
   cerr << "Completing" << std::endl;
   bstore->mount();