From 7a73d9b155bdd9e8cfffbf4968e57e4fbd95ac65 Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@redhat.com>
Date: Thu, 22 Feb 2018 13:15:17 -0600
Subject: [PATCH] os/bluestore: bluestore_cache_kv_max ->
 bluestore_cache_kv_min

Currently, the default tunables are
  cache_meta_ratio = .01
  cache_kv_ratio = .99
  cache_kv_max = 512MB

We currently have no doubts that, for low memory situations, we want to
give all of the memory to rocksdb.  The .99/.01 values are an indirect
way to communicating that.  However, we do see problems with high-memory
situations where rocksdb needs more than 512MB (due to, we believe,
bloom filters). And it is very reasonable to think that there are
situations and reasons why we might want more than 512MB for rocksdb.

So, we change kv_max -> kv_min.  For low memory situations, we give *all*
memory to rocksdb.  (If you don't want this, set kv_min = 0.)  Above that,
respect the kv_ratio and meta_ratio settings.

I don't see much reason to have an absolute kv_max on the rocksdb memory.

We adjust the kv_ratio and meta_ratio defaults in this patch so that all
high memory goes to bluestore and there is no effective change in
default behavior for this patch.

Signed-off-by: Sage Weil <sage@redhat.com>
---
 src/common/legacy_config_opts.h |  2 +-
 src/common/options.cc           | 15 ++++++++-----
 src/os/bluestore/BlueStore.cc   | 38 +++++++++++++++++----------------
 3 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/src/common/legacy_config_opts.h b/src/common/legacy_config_opts.h
index 77828795fe6db..cb3aeadbc94fa 100644
--- a/src/common/legacy_config_opts.h
+++ b/src/common/legacy_config_opts.h
@@ -1034,7 +1034,7 @@ OPTION(bluestore_cache_size_hdd, OPT_U64)
 OPTION(bluestore_cache_size_ssd, OPT_U64)
 OPTION(bluestore_cache_meta_ratio, OPT_DOUBLE)
 OPTION(bluestore_cache_kv_ratio, OPT_DOUBLE)
-OPTION(bluestore_cache_kv_max, OPT_INT) // limit the maximum amount of cache for the kv store
+OPTION(bluestore_cache_kv_min, OPT_INT)
 OPTION(bluestore_kvbackend, OPT_STR)
 OPTION(bluestore_allocator, OPT_STR)     // stupid | bitmap
 OPTION(bluestore_freelist_blocks_per_key, OPT_INT)
diff --git a/src/common/options.cc b/src/common/options.cc
index 6d012c8a5edd1..150042503e1d2 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -3735,19 +3735,24 @@ std::vector<Option> get_global_options() {
     .set_description("Default bluestore_cache_size for non-rotational (solid state) media"),
 
     Option("bluestore_cache_meta_ratio", Option::TYPE_FLOAT, Option::LEVEL_ADVANCED)
-    .set_default(.01)
+    .set_default(.99)
+    .add_see_also("bluestore_cache_size")
     .set_description("Ratio of bluestore cache to devote to metadata"),
 
     Option("bluestore_cache_kv_ratio", Option::TYPE_FLOAT, Option::LEVEL_ADVANCED)
-    .set_default(.99)
+    .set_default(.01)
+    .add_see_also("bluestore_cache_size")
     .set_description("Ratio of bluestore cache to devote to kv database (rocksdb)"),
 
-    Option("bluestore_cache_kv_max", Option::TYPE_INT, Option::LEVEL_ADVANCED)
+    Option("bluestore_cache_kv_min", Option::TYPE_INT, Option::LEVEL_ADVANCED)
     .set_default(512_M)
-    .set_description("Max memory (bytes) to devote to kv database (rocksdb)")
+    .set_description("Minimum memory (bytes) of bluestore_cache_size to devote to kv database (rocksdb)")
+    .add_see_also("bluestore_cache_size")
     .set_long_description("A negative value means using bluestore_cache_meta_ratio "
       "and bluestore_cache_kv_ratio instead of calculating these ratios using "
-      "bluestore_cache_size_* and bluestore_cache_kv_max."),
+      "bluestore_cache_size_* and bluestore_cache_kv_min.  If "
+      "bluestore_cache_size is below bluestore_cache_kv_min "
+      "then this option has no effect."),
 
     Option("bluestore_kvbackend", Option::TYPE_STR, Option::LEVEL_DEV)
     .set_default("rocksdb")
diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index e4cc918219e70..279d7db3ae72a 100644
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -3869,24 +3869,6 @@ int BlueStore::_set_cache_sizes()
   cache_meta_ratio = cct->_conf->bluestore_cache_meta_ratio;
   cache_kv_ratio = cct->_conf->bluestore_cache_kv_ratio;
 
-  double cache_kv_max = cct->_conf->bluestore_cache_kv_max;
-  double cache_kv_max_ratio = 0;
-
-  // if cache_kv_max is negative, disable it
-  if (cache_size > 0 && cache_kv_max >= 0) {
-    cache_kv_max_ratio = (double) cache_kv_max / (double) cache_size;
-    if (cache_kv_max_ratio < 1.0 && cache_kv_max_ratio < cache_kv_ratio) {
-      dout(1) << __func__ << " max " << cache_kv_max_ratio
-            << " < ratio " << cache_kv_ratio
-            << dendl;
-      cache_meta_ratio = cache_meta_ratio + cache_kv_ratio - cache_kv_max_ratio;
-      cache_kv_ratio = cache_kv_max_ratio;
-    }
-  }  
-
-  cache_data_ratio =
-    (double)1.0 - (double)cache_meta_ratio - (double)cache_kv_ratio;
-
   if (cache_meta_ratio < 0 || cache_meta_ratio > 1.0) {
     derr << __func__ << " bluestore_cache_meta_ratio (" << cache_meta_ratio
 	 << ") must be in range [0,1.0]" << dendl;
@@ -3904,6 +3886,26 @@ int BlueStore::_set_cache_sizes()
 	 << dendl;
     return -EINVAL;
   }
+
+  double cache_kv_min = cct->_conf->bluestore_cache_kv_min;
+  double cache_kv_min_ratio = 0;
+
+  // if cache_kv_min is negative, disable it
+  if (cache_size > 0 && cache_kv_min >= 0) {
+    cache_kv_min_ratio = std::min((double)cache_kv_min / (double)cache_size,
+				  (double)1.0);
+    if (cache_kv_min_ratio > cache_kv_ratio) {
+      dout(1) << __func__ << " kv_min_ratio " << cache_kv_min_ratio
+            << " > kv_ratio " << cache_kv_ratio << dendl;
+      cache_kv_ratio = cache_kv_min_ratio;
+      cache_meta_ratio = std::min((double)cache_meta_ratio,
+				  (double)1.0 - cache_kv_ratio);
+    }
+  }
+
+  cache_data_ratio =
+    (double)1.0 - (double)cache_meta_ratio - (double)cache_kv_ratio;
+
   if (cache_data_ratio < 0) {
     // deal with floating point imprecision
     cache_data_ratio = 0;
-- 
2.39.5