From 35c0d7588861ea9fc69789bf4ac0affee8fafd5f Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@redhat.com>
Date: Thu, 20 Jun 2019 10:50:13 -0500
Subject: [PATCH] osd: add hdd and ssd variants for osd_recovery_max_active

Semi-arbitrarily set the SSD max to 10 (instead of 3).  This should be
tuned based on some real data.

Signed-off-by: Sage Weil <sage@redhat.com>
---
 PendingReleaseNotes                        | 10 ++++++++++
 doc/rados/configuration/osd-config-ref.rst | 21 +++++++++++++++++++++
 src/common/legacy_config_opts.h            |  2 ++
 src/common/options.cc                      | 16 +++++++++++++++-
 src/osd/OSD.cc                             | 17 ++++++++++++++---
 src/osd/OSD.h                              |  2 ++
 6 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index d433e2f4ae3..abecf9ace7e 100644
--- a/PendingReleaseNotes
+++ b/PendingReleaseNotes
@@ -86,3 +86,13 @@
   parsing date or time values from the unstructured human-readable
   output should be modified to parse the structured output instead, as
   the human-readable output may change without notice.
+
+* The ``osd_recovery_max_active`` option now has
+  ``osd_recovery_max_active_hdd`` and ``osd_recovery_max_active_ssd``
+  variants, each with different default values for HDD and SSD-backed
+  OSDs, respectively.  By default ``osd_recovery_max_active`` now
+  defaults to zero, which means that the OSD will conditionally use
+  the HDD or SSD option values.  Administrators who have customized
+  this value may want to consider whether they have set this to a
+  value similar to the new defaults (3 for HDDs and 10 for SSDs) and,
+  if so, remove the option from their configuration entirely.
\ No newline at end of file
diff --git a/doc/rados/configuration/osd-config-ref.rst b/doc/rados/configuration/osd-config-ref.rst
index edcd91e5607..6afc1cc4f1d 100644
--- a/doc/rados/configuration/osd-config-ref.rst
+++ b/doc/rados/configuration/osd-config-ref.rst
@@ -915,9 +915,30 @@ perform well in a degraded state.
               requests will accelerate recovery, but the requests places an
               increased load on the cluster.
 
+	      This value is only used if it is non-zero. Normally it
+	      is ``0``, which means that the ``hdd`` or ``ssd`` values
+	      (below) are used, depending on the type of the primary
+	      device backing the OSD.
+
+:Type: 32-bit Integer
+:Default: ``0``
+
+``osd recovery max active hdd``
+
+:Description: The number of active recovery requests per OSD at one time, if the
+	      primary device is rotational.
+
 :Type: 32-bit Integer
 :Default: ``3``
 
+``osd recovery max active ssd``
+
+:Description: The number of active recovery requests per OSD at one time, if the
+	      priary device is non-rotational (i.e., an SSD).
+
+:Type: 32-bit Integer
+:Default: ``10``
+
 
 ``osd recovery max chunk``
 
diff --git a/src/common/legacy_config_opts.h b/src/common/legacy_config_opts.h
index d2fa55af353..f6d95704186 100644
--- a/src/common/legacy_config_opts.h
+++ b/src/common/legacy_config_opts.h
@@ -689,6 +689,8 @@ OPTION(osd_default_data_pool_replay_window, OPT_INT)
 OPTION(osd_auto_mark_unfound_lost, OPT_BOOL)
 OPTION(osd_recovery_delay_start, OPT_FLOAT)
 OPTION(osd_recovery_max_active, OPT_U64)
+OPTION(osd_recovery_max_active_hdd, OPT_U64)
+OPTION(osd_recovery_max_active_ssd, OPT_U64)
 OPTION(osd_recovery_max_single_start, OPT_U64)
 OPTION(osd_recovery_max_chunk, OPT_U64)  // max size of push chunk
 OPTION(osd_recovery_max_omap_entries_per_chunk, OPT_U64) // max number of omap entries per chunk; 0 to disable limit
diff --git a/src/common/options.cc b/src/common/options.cc
index 2b02dbfa907..8f9c899e6ee 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -3352,8 +3352,22 @@ std::vector<Option> get_global_options() {
     .set_description(""),
 
     Option("osd_recovery_max_active", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
+    .set_default(0)
+    .set_description("Number of simultaneous active recovery operations per OSD (overrides _ssd and _hdd if non-zero)")
+    .add_see_also("osd_recovery_max_active_hdd")
+    .add_see_also("osd_recovery_max_active_ssd"),
+
+    Option("osd_recovery_max_active_hdd", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
     .set_default(3)
-    .set_description(""),
+    .set_description("Number of simultaneous active recovery oeprations per OSD (for rotational devices)")
+    .add_see_also("osd_recovery_max_active")
+    .add_see_also("osd_recovery_max_active_ssd"),
+
+    Option("osd_recovery_max_active_ssd", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
+    .set_default(10)
+    .set_description("Number of simultaneous active recovery oeprations per OSD (for non-rotational solid state devices)")
+    .add_see_also("osd_recovery_max_active")
+    .add_see_also("osd_recovery_max_active_hdd"),
 
     Option("osd_recovery_max_single_start", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
     .set_default(1)
diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index 155ddd3b827..67c0834d587 100644
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -2689,6 +2689,16 @@ float OSD::get_osd_delete_sleep()
   return cct->_conf.get_val<double>("osd_delete_sleep_hdd");
 }
 
+int OSD::get_recovery_max_active()
+{
+  if (cct->_conf->osd_recovery_max_active)
+    return cct->_conf->osd_recovery_max_active;
+  if (store_is_rotational)
+    return cct->_conf->osd_recovery_max_active_hdd;
+  else
+    return cct->_conf->osd_recovery_max_active_ssd;
+}
+
 int OSD::init()
 {
   CompatSet initial, diff;
@@ -9055,7 +9065,7 @@ bool OSDService::_recover_now(uint64_t *available_pushes)
     return false;
   }
 
-  uint64_t max = cct->_conf->osd_recovery_max_active;
+  uint64_t max = osd->get_recovery_max_active();
   if (max <= recovery_ops_active + recovery_ops_reserved) {
     dout(15) << __func__ << " active " << recovery_ops_active
 	     << " + reserved " << recovery_ops_reserved
@@ -9148,7 +9158,7 @@ void OSDService::start_recovery_op(PG *pg, const hobject_t& soid)
   std::lock_guard l(recovery_lock);
   dout(10) << "start_recovery_op " << *pg << " " << soid
 	   << " (" << recovery_ops_active << "/"
-	   << cct->_conf->osd_recovery_max_active << " rops)"
+	   << osd->get_recovery_max_active() << " rops)"
 	   << dendl;
   recovery_ops_active++;
 
@@ -9164,7 +9174,8 @@ void OSDService::finish_recovery_op(PG *pg, const hobject_t& soid, bool dequeue)
   std::lock_guard l(recovery_lock);
   dout(10) << "finish_recovery_op " << *pg << " " << soid
 	   << " dequeue=" << dequeue
-	   << " (" << recovery_ops_active << "/" << cct->_conf->osd_recovery_max_active << " rops)"
+	   << " (" << recovery_ops_active << "/"
+	   << osd->get_recovery_max_active() << " rops)"
 	   << dendl;
 
   // adjust count
diff --git a/src/osd/OSD.h b/src/osd/OSD.h
index b2a9df58e8e..40b6a07eacd 100644
--- a/src/osd/OSD.h
+++ b/src/osd/OSD.h
@@ -2119,6 +2119,8 @@ private:
   float get_osd_recovery_sleep();
   float get_osd_delete_sleep();
 
+  int get_recovery_max_active();
+
   void probe_smart(const string& devid, ostream& ss);
 
 public:
-- 
2.39.5