From a15efa16211c50b5f1e0eda116dae2b060162048 Mon Sep 17 00:00:00 2001
From: Igor Fedotov <ifedotov@suse.com>
Date: Wed, 19 Dec 2018 01:10:02 +0300
Subject: [PATCH] os/bluestore: more robust handling for lack of per-pool stats
 cases.

Fixes: https://tracker.ceph.com/issues/37652

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
---
 src/common/legacy_config_opts.h    |  3 +-
 src/common/options.cc              | 17 ++++++-----
 src/os/bluestore/BlueStore.cc      | 49 +++++++++++++++++-------------
 src/os/bluestore/BlueStore.h       |  7 +++--
 src/test/objectstore/store_test.cc |  6 ++--
 5 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/src/common/legacy_config_opts.h b/src/common/legacy_config_opts.h
index 270f1ec50f5..b1996b79da2 100644
--- a/src/common/legacy_config_opts.h
+++ b/src/common/legacy_config_opts.h
@@ -1042,7 +1042,6 @@ OPTION(bluestore_fsck_on_umount, OPT_BOOL)
 OPTION(bluestore_fsck_on_umount_deep, OPT_BOOL)
 OPTION(bluestore_fsck_on_mkfs, OPT_BOOL)
 OPTION(bluestore_fsck_on_mkfs_deep, OPT_BOOL)
-OPTION(bluestore_fsck_error_on_legacy_stats, OPT_BOOL)
 OPTION(bluestore_sync_submit_transaction, OPT_BOOL) // submit kv txn in queueing thread (not kv_sync_thread)
 OPTION(bluestore_throttle_bytes, OPT_U64)
 OPTION(bluestore_throttle_deferred_bytes, OPT_U64)
@@ -1072,7 +1071,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_debug_no_per_pool_stats, OPT_BOOL)
+OPTION(bluestore_no_per_pool_stats_tolerance, OPT_STR)
 
 OPTION(kstore_max_ops, OPT_U64)
 OPTION(kstore_max_bytes, OPT_U64)
diff --git a/src/common/options.cc b/src/common/options.cc
index 4e9b426aadc..ceb9a709836 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -4459,10 +4459,6 @@ std::vector<Option> get_global_options() {
     .set_default(false)
     .set_description("Run deep fsck after mkfs"),
 
-    Option("bluestore_fsck_error_on_legacy_stats", Option::TYPE_BOOL, Option::LEVEL_DEV)
-    .set_default(false)
-    .set_description("Popup errors on legacy (store-wide only) stats detection"),
-
     Option("bluestore_sync_submit_transaction", Option::TYPE_BOOL, Option::LEVEL_DEV)
     .set_default(false)
     .set_description("Try to submit metadata transaction to rocksdb in queuing thread context"),
@@ -4591,10 +4587,15 @@ std::vector<Option> get_global_options() {
     .set_default(0.0)
     .set_description("inject crc verification errors into bluestore device reads"),
 
-    Option("bluestore_debug_no_per_pool_stats", Option::TYPE_BOOL, Option::LEVEL_DEV)
-    .set_default(false)
-    .set_description(""),
-
+    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."),
     // -----------------------------------------
     // kstore
 
diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 08c6f57907a..db402b045e6 100644
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -5572,7 +5572,7 @@ void BlueStore::_open_statfs()
     } else {
       dout(10) << __func__ << " store_statfs is corrupt, using empty" << dendl;
     }
-  } else if (cct->_conf->bluestore_debug_no_per_pool_stats) {
+  } 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 {
@@ -6499,24 +6499,26 @@ int BlueStore::_fsck_check_extents(
 
 void BlueStore::_fsck_check_pool_statfs(
   BlueStore::per_pool_statfs& expected_pool_statfs,
+  bool need_per_pool_stats,
   int& errors,
   BlueStoreRepairer* repairer)
 {
-  if (!per_pool_stat_collection) {
-    return;
-  }
   auto it = db->get_iterator(PREFIX_STAT);
   if (it) {
     for (it->lower_bound(string()); it->valid(); it->next()) {
       string key = it->key();
       if (key == BLUESTORE_GLOBAL_STATFS_KEY) {
         if (repairer) {
-	  derr << "fsck error: legacy statfs record found, removing" << dendl;
-	  repairer->remove_key(db, PREFIX_STAT, BLUESTORE_GLOBAL_STATFS_KEY);
-	  errors++;
+	  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;
+	  }
 	} else {
 	  const char* s = "fsck warning: ";
-          if (cct->_conf->bluestore_fsck_error_on_legacy_stats) {
+          if (need_per_pool_stats) {
 	    ++errors;
 	    s = "fsck error: ";
 	  }
@@ -6526,6 +6528,9 @@ void BlueStore::_fsck_check_pool_statfs(
 	}
 	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
@@ -6690,6 +6695,10 @@ int BlueStore::_fsck(bool deep, bool repair)
   store_statfs_t* expected_statfs = nullptr;
 
   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";
 
   int r = _open_path();
   if (r < 0)
@@ -6800,11 +6809,7 @@ int BlueStore::_fsck(bool deep, bool repair)
   actual_statfs.internal_metadata = 0;
   actual_statfs.omap_allocated = 0;
 
-  // switch to per-pool stats if not explicitly prohibited
-  if (!per_pool_stat_collection &&
-        !cct->_conf->bluestore_debug_no_per_pool_stats) {
-    per_pool_stat_collection = true;
-  }
+  need_per_pool_stats = per_pool_stat_collection || need_per_pool_stats;
 
   // walk PREFIX_OBJ
   dout(1) << __func__ << " walking object keyspace" << dendl;
@@ -6891,7 +6896,7 @@ int BlueStore::_fsck(bool deep, bool repair)
 	auto pool_id = c->cid.is_pg(&pgid) ? pgid.pool() : META_POOL_ID;
 	dout(20) << __func__ << "  collection " << c->cid << " " << c->cnode
 		 << dendl;
-	if (per_pool_stat_collection) {
+	if (need_per_pool_stats) {
 	  expected_statfs = &expected_pool_statfs[pool_id];
 	}
 
@@ -7178,7 +7183,7 @@ int BlueStore::_fsck(bool deep, bool repair)
 	for (auto &r : shared_blob.ref_map.ref_map) {
 	  extents.emplace_back(bluestore_pextent_t(r.first, r.second.length));
 	}
-	if (per_pool_stat_collection) {
+	if (need_per_pool_stats) {
 	  expected_statfs = &expected_pool_statfs[sbi.pool_id];
 	}
 	errors += _fsck_check_extents(sbi.cid,
@@ -7238,7 +7243,7 @@ int BlueStore::_fsck(bool deep, bool repair)
 	    continue;
 	  }
 	  auto pool_id = c->cid.is_pg(&pgid) ? pgid.pool() : META_POOL_ID;
-	  if (per_pool_stat_collection) {
+	  if (need_per_pool_stats) {
 	    expected_statfs = &expected_pool_statfs[pool_id];
 	  }
 	}
@@ -7416,7 +7421,8 @@ int BlueStore::_fsck(bool deep, bool repair)
   }
   sb_info.clear();
 
-  if (!per_pool_stat_collection) {
+  // 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;
@@ -7426,10 +7432,11 @@ int BlueStore::_fsck(bool deep, bool repair)
       }
       ++errors;
     }
-  } else {
+  }
+  if (!enforce_no_per_pool_stats) {
     dout(1) << __func__ << " checking pool_statfs" << dendl;
-    _fsck_check_pool_statfs(expected_pool_statfs, errors,
-      repair ? &repairer : nullptr);
+    _fsck_check_pool_statfs(expected_pool_statfs, need_per_pool_stats,
+      errors, repair ? &repairer : nullptr);
   }
 
   dout(1) << __func__ << " checking for stray omap data" << dendl;
@@ -9434,7 +9441,7 @@ void BlueStore::_txc_update_store_statfs(TransContext *txc)
     vstatfs += txc->statfs_delta; //non-persistent in this mode
 
   } else {
-    txc->t->merge(PREFIX_STAT, "bluestore_statfs", bl);
+    txc->t->merge(PREFIX_STAT, BLUESTORE_GLOBAL_STATFS_KEY, bl);
 
     std::lock_guard l(vstatfs_lock);
     vstatfs += txc->statfs_delta;
diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index cea062a84e9..a263257785d 100644
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -2278,8 +2278,11 @@ private:
 
   using  per_pool_statfs =
     mempool::bluestore_fsck::map<uint64_t, store_statfs_t>;
-  void _fsck_check_pool_statfs(per_pool_statfs& expected_pool_statfs,
-    int& errors, BlueStoreRepairer* repairer);
+  void _fsck_check_pool_statfs(
+    per_pool_statfs& expected_pool_statfs,
+    bool need_per_pool_stats,
+    int& errors,
+    BlueStoreRepairer* repairer);
 
   void _buffer_cache_write(
     TransContext *txc,
diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc
index e718ada1260..862534d0857 100644
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -7287,8 +7287,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_debug_no_per_pool_stats", "true");
-  SetVal(g_conf(), "bluestore_fsck_error_on_legacy_stats", "true");
+  SetVal(g_conf(), "bluestore_no_per_pool_stats_tolerance", "enforce");
 
   StartDeferred(0x10000);
 
@@ -7432,14 +7431,13 @@ TEST_P(StoreTestSpecificAUSize, BluestoreRepairTest) {
 
   // enable per-pool stats collection hence causing fsck to fail
   cerr << "per-pool statfs" << std::endl;
-  SetVal(g_conf(), "bluestore_debug_no_per_pool_stats", "false");
+  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();
 
-- 
2.39.5