mon: PaxosService: fix trim criteria so to avoid constantly trimming

Say a service establishes it will only keep 500 versions once a given
condition X is true.  Now say that said condition X only becomes true
after said service committing some 800 versions.

Once we decide to trim, this service would trim all 300 surplus versions
in one go.  After that, each committed version would also trim the
previous version.

Trimming an unbounded number of versions is not a good practice
as it will generate bigger transactions (thus a greater workload on
leveldb) and therefore bigger messages too.

Constantly trimming versions implies more frequent accesses to leveldb,
and keeping around a couple more versions won't hurt us in any significant
way, so let us put off trimming unless we go over a predefined minimum.

This patch adds two new options:

 paxos service trim min - minimum amount of versions to trigger a trim
                          (default: 30, 0 disables it)
 paxos service trim max - maximum amount of versions to trim during a
                          single proposal
                          (default: 50, 0 disables it)

Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>

6 files changed, 38 insertions, 7 deletions

diff --git a/src/common/config_opts.h b/src/common/config_opts.h
index 3a9cfd70827..a0a21ca5b1c 100644
--- a/src/common/config_opts.h
+++ b/src/common/config_opts.h
@@ -191,6 +191,8 @@ OPTION(paxos_propose_interval, OPT_DOUBLE, 1.0)  // gather updates for this long
 OPTION(paxos_min_wait, OPT_DOUBLE, 0.05)  // min time to gather updates for after period of inactivity
 OPTION(paxos_trim_tolerance, OPT_INT, 30) // number of extra proposals tolerated before trimming
 OPTION(paxos_trim_disabled_max_versions, OPT_INT, 100) // maximum amount of versions we shall allow passing by without trimming
+OPTION(paxos_service_trim_max, OPT_INT, 50) // maximum amount of versions to trim during a single proposal (0 disables it)
+OPTION(paxos_service_trim_min, OPT_INT, 30) // minimum amount of versions to trigger a trim (0 disables it)
 OPTION(clock_offset, OPT_DOUBLE, 0) // how much to offset the system clock in Clock.cc
 OPTION(auth_cluster_required, OPT_STR, "cephx")   // required of mon, mds, osd daemons
 OPTION(auth_service_required, OPT_STR, "cephx")   // required by daemons of clients
diff --git a/src/mon/MDSMonitor.h b/src/mon/MDSMonitor.h
index 3b1fbf92e2d..bda8f53c4fa 100644
--- a/src/mon/MDSMonitor.h
+++ b/src/mon/MDSMonitor.h
@@ -75,7 +75,7 @@ class MDSMonitor : public PaxosService {
   // we don't require full versions; don't encode any.
   virtual void encode_full(MonitorDBStore::Transaction *t) { }
 
-  bool should_trim() { return false; }
+  bool service_should_trim() { return false; }
   void encode_trim(MonitorDBStore::Transaction *t) { }
 
   void update_logger();
diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc
index 40103ec402f..21eba6c83ec 100644
--- a/src/mon/OSDMonitor.cc
+++ b/src/mon/OSDMonitor.cc
@@ -536,7 +536,7 @@ void OSDMonitor::update_trim()
   }
 }
 
-bool OSDMonitor::should_trim()
+bool OSDMonitor::service_should_trim()
 {
   update_trim();
   return (get_trim_to() > 0);
diff --git a/src/mon/OSDMonitor.h b/src/mon/OSDMonitor.h
index e9f72298eb6..036aed5ffd3 100644
--- a/src/mon/OSDMonitor.h
+++ b/src/mon/OSDMonitor.h
@@ -155,7 +155,7 @@ private:
   bool should_propose(double &delay);
 
   void update_trim();
-  bool should_trim();
+  bool service_should_trim();
 
   bool can_mark_down(int o);
   bool can_mark_up(int o);
diff --git a/src/mon/PaxosService.cc b/src/mon/PaxosService.cc
index a66c5ec0612..44d53c20723 100644
--- a/src/mon/PaxosService.cc
+++ b/src/mon/PaxosService.cc
@@ -152,7 +152,6 @@ void PaxosService::propose_pending()
 
   if (should_trim()) {
     encode_trim(&t);
-    set_trim_to(0);
   }
 
   encode_pending(&t);
@@ -327,7 +326,19 @@ void PaxosService::encode_trim(MonitorDBStore::Transaction *t)
   if (first_committed >= trim_to)
     return;
 
-  trim(t, first_committed, trim_to);
-  put_first_committed(t, trim_to);
+  version_t trim_to_max = trim_to;
+  if ((g_conf->paxos_service_trim_max > 0)
+      && (trim_to - first_committed > (size_t)g_conf->paxos_service_trim_max)) {
+    trim_to_max = first_committed + g_conf->paxos_service_trim_max;
+  }
+
+  dout(10) << __func__ << " trimming versions " << first_committed
+           << " to " << trim_to_max << dendl;
+
+  trim(t, first_committed, trim_to_max);
+  put_first_committed(t, trim_to_max);
+
+  if (trim_to_max == trim_to)
+    set_trim_to(0);
 }
 
diff --git a/src/mon/PaxosService.h b/src/mon/PaxosService.h
index f16bc77cdeb..d3253b02c70 100644
--- a/src/mon/PaxosService.h
+++ b/src/mon/PaxosService.h
@@ -600,6 +600,24 @@ public:
    */
   virtual void encode_trim(MonitorDBStore::Transaction *t);
   /**
+   *
+   */
+  virtual bool should_trim() {
+    bool want_trim = service_should_trim();
+
+    if (!want_trim)
+      return false;
+
+    if (g_conf->paxos_service_trim_min > 0) {
+      version_t trim_to = get_trim_to();
+      version_t first = get_first_committed();
+
+      if ((trim_to > 0) && trim_to > first)
+        return ((trim_to - first) >= (version_t)g_conf->paxos_service_trim_min);
+    }
+    return true;
+  }
+  /**
    * Check if we should trim.
    *
    * We define this function here, because we assume that as long as we know of
@@ -608,7 +626,7 @@ public:
    *
    * @returns true if we should trim; false otherwise.
    */
-  virtual bool should_trim() {
+  virtual bool service_should_trim() {
     update_trim();
     return (get_trim_to() > 0);
   }