From 05f7294558ef380e2d9fd2deb848be2570a07854 Mon Sep 17 00:00:00 2001
From: Igor Fedotov <ifedotov@suse.com>
Date: Fri, 26 Feb 2021 17:16:11 +0300
Subject: [PATCH] os/bluestore: go beyond pinned onodes while trimming the
 cache.

One might face lack of cache trimming when there is a bunch of pinned entries on the top of Onode's cache LRU list. If these pinned entries stay in the state for a long time cache might start using too much memory causing OSD to go out of osd-memory-target limit. Pinned state tend to happen to osdmap onodes.
The proposed patch preserves last trim position in the LRU list (if it pointed to a pinned entry) and proceeds trimming from that position if it wasn't invalidated. LRU nature of the list enables to do that safely since no new entries appear above the previously present entry while it's not touched.

Fixes: https://tracker.ceph.com/issues/48729
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
---
 src/common/legacy_config_opts.h    |   1 +
 src/common/options.cc              |   6 +-
 src/os/bluestore/BlueStore.cc      |  38 +++++---
 src/os/bluestore/BlueStore.h       |  23 ++++-
 src/test/objectstore/store_test.cc | 138 +++++++++++++++++++++++++++++
 5 files changed, 188 insertions(+), 18 deletions(-)

diff --git a/src/common/legacy_config_opts.h b/src/common/legacy_config_opts.h
index 430ef684848..37629afd159 100644
--- a/src/common/legacy_config_opts.h
+++ b/src/common/legacy_config_opts.h
@@ -1075,6 +1075,7 @@ OPTION(bluestore_default_buffered_write, OPT_BOOL)
 OPTION(bluestore_debug_misc, OPT_BOOL)
 OPTION(bluestore_debug_no_reuse_blocks, OPT_BOOL)
 OPTION(bluestore_debug_small_allocations, OPT_INT)
+OPTION(bluestore_debug_max_cached_onodes, OPT_INT)
 OPTION(bluestore_debug_too_many_blobs_threshold, OPT_INT)
 OPTION(bluestore_debug_freelist, OPT_BOOL)
 OPTION(bluestore_debug_prefill, OPT_FLOAT)
diff --git a/src/common/options.cc b/src/common/options.cc
index c0c9f06ab17..c769bd0bc16 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -4914,7 +4914,11 @@ std::vector<Option> get_global_options() {
     Option("bluestore_debug_small_allocations", Option::TYPE_INT, Option::LEVEL_DEV)
     .set_default(0)
     .set_description(""),
-
+    Option("bluestore_debug_max_cached_onodes", Option::TYPE_INT, Option::LEVEL_DEV)
+    .set_default(0)
+    .set_description("This allows to explicitly cap number of onode entries per cache shard "
+                     "effectively bypassing all the smart but indirect cache adjustment logic."
+                     " Intended for testing purposes only "),
     Option("bluestore_debug_too_many_blobs_threshold", Option::TYPE_INT, Option::LEVEL_DEV)
     .set_default(24*1024)
     .set_description(""),
diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 2697b514e0b..a1c83ceadf4 100644
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -1170,12 +1170,14 @@ void BlueStore::LRUCache::_trim(uint64_t onode_max, uint64_t buffer_max)
   }
 
   // onodes
-  if (onode_max >= onode_lru.size()) {
+  if (onode_max >= onode_lru.size() ||
+      last_pinned == onode_lru.begin()) {
     return; // don't even try
   }
   uint64_t num = onode_lru.size() - onode_max;
 
-  auto p = onode_lru.end();
+  auto p = last_pinned;
+  last_pinned = onode_lru.end();
   ceph_assert(p != onode_lru.begin());
   --p;
   int skipped = 0;
@@ -1187,8 +1189,9 @@ void BlueStore::LRUCache::_trim(uint64_t onode_max, uint64_t buffer_max)
       dout(20) << __func__ << "  " << o->oid << " has " << refs
 	       << " refs, skipping" << dendl;
       if (++skipped >= max_skipped) {
-        dout(20) << __func__ << " maximum skip pinned reached; stopping with "
+        dout(15) << __func__ << " maximum skip pinned reached; stopping with "
                  << num << " left to trim" << dendl;
+	last_pinned = p;
         break;
       }
 
@@ -1202,10 +1205,11 @@ void BlueStore::LRUCache::_trim(uint64_t onode_max, uint64_t buffer_max)
     }
     dout(30) << __func__ << "  rm " << o->oid << dendl;
     if (p != onode_lru.begin()) {
-      onode_lru.erase(p--);
+      _onode_lru_erase(p--);
     } else {
-      onode_lru.erase(p);
-      ceph_assert(num == 1);
+      _onode_lru_erase(p);
+      num = 1; // fake num to end the loop
+               // in we might still have some pinned onodes
     }
     o->get();  // paranoia
     o->c->onode_map.remove(o->oid);
@@ -1243,7 +1247,7 @@ void BlueStore::LRUCache::_audit(const char *when)
 void BlueStore::TwoQCache::_touch_onode(OnodeRef& o)
 {
   auto p = onode_lru.iterator_to(*o);
-  onode_lru.erase(p);
+  _onode_lru_erase(p);
   onode_lru.push_front(*o);
 }
 
@@ -1467,12 +1471,14 @@ void BlueStore::TwoQCache::_trim(uint64_t onode_max, uint64_t buffer_max)
   }
 
   // onodes
-  if (onode_max >= onode_lru.size()) {
+  if (onode_max >= onode_lru.size() ||
+     last_pinned == onode_lru.begin()) {
     return; // don't even try
   }
   uint64_t num = onode_lru.size() - onode_max;
 
-  auto p = onode_lru.end();
+  auto p = last_pinned;
+  last_pinned = onode_lru.end();
   ceph_assert(p != onode_lru.begin());
   --p;
   int skipped = 0;
@@ -1485,8 +1491,9 @@ void BlueStore::TwoQCache::_trim(uint64_t onode_max, uint64_t buffer_max)
       dout(20) << __func__ << "  " << o->oid << " has " << refs
 	       << " refs; skipping" << dendl;
       if (++skipped >= max_skipped) {
-        dout(20) << __func__ << " maximum skip pinned reached; stopping with "
+        dout(15) << __func__ << " maximum skip pinned reached; stopping with "
                  << num << " left to trim" << dendl;
+        last_pinned = p;
         break;
       }
 
@@ -1500,10 +1507,11 @@ void BlueStore::TwoQCache::_trim(uint64_t onode_max, uint64_t buffer_max)
     }
     dout(30) << __func__ << " " << o->oid << " num=" << num <<" lru size="<<onode_lru.size()<< dendl;
     if (p != onode_lru.begin()) {
-      onode_lru.erase(p--);
+      _onode_lru_erase(p--);
     } else {
-      onode_lru.erase(p);
-      ceph_assert(num == 1);
+      _onode_lru_erase(p);
+      num = 1; // fake num to end the loop
+               // in we might still have some pinned onodes
     }
     o->get();  // paranoia
     o->c->onode_map.remove(o->oid);
@@ -3970,6 +3978,10 @@ void BlueStore::MempoolThread::_trim_shards(bool interval_stats)
       (meta_alloc / (double) num_shards) / meta_cache->get_bytes_per_onode());
   uint64_t max_shard_buffer = static_cast<uint64_t>(data_alloc / num_shards);
 
+  auto debug_max_onodes = g_conf()->bluestore_debug_max_cached_onodes;
+  if (debug_max_onodes) {
+    max_shard_onodes = debug_max_onodes;
+  }
   ldout(cct, 30) << __func__ << " max_shard_onodes: " << max_shard_onodes
                  << " max_shard_buffer: " << max_shard_buffer << dendl;
 
diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index d64d5584c2b..0db06063c24 100644
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -1192,12 +1192,20 @@ public:
 	&Buffer::lru_item> > buffer_lru_list_t;
 
     onode_lru_list_t onode_lru;
+    onode_lru_list_t::iterator last_pinned;
 
     buffer_lru_list_t buffer_lru;
     uint64_t buffer_size = 0;
 
+    void _onode_lru_erase(onode_lru_list_t::iterator it) {
+      if (it == last_pinned) {
+        last_pinned = onode_lru.end();
+      }
+      onode_lru.erase(it);
+    }
+
   public:
-    LRUCache(CephContext* cct) : Cache(cct) {}
+    LRUCache(CephContext* cct) : Cache(cct), last_pinned(onode_lru.end()){}
     uint64_t _get_num_onodes() override {
       return onode_lru.size();
     }
@@ -1209,7 +1217,7 @@ public:
     }
     void _rm_onode(OnodeRef& o) override {
       auto q = onode_lru.iterator_to(*o);
-      onode_lru.erase(q);
+      _onode_lru_erase(q);
     }
     void _touch_onode(OnodeRef& o) override;
 
@@ -1285,6 +1293,7 @@ public:
 	&Buffer::lru_item> > buffer_list_t;
 
     onode_lru_list_t onode_lru;
+    onode_lru_list_t::iterator last_pinned;
 
     buffer_list_t buffer_hot;      ///< "Am" hot buffers
     buffer_list_t buffer_warm_in;  ///< "A1in" newly warm buffers
@@ -1301,8 +1310,14 @@ public:
 
     uint64_t buffer_list_bytes[BUFFER_TYPE_MAX] = {0}; ///< bytes per type
 
+    void _onode_lru_erase(onode_lru_list_t::iterator it) {
+      if (it == last_pinned) {
+        last_pinned = onode_lru.end();
+      }
+      onode_lru.erase(it);
+    }
   public:
-    TwoQCache(CephContext* cct) : Cache(cct) {}
+    TwoQCache(CephContext* cct) : Cache(cct), last_pinned(onode_lru.end()){}
     uint64_t _get_num_onodes() override {
       return onode_lru.size();
     }
@@ -1314,7 +1329,7 @@ public:
     }
     void _rm_onode(OnodeRef& o) override {
       auto q = onode_lru.iterator_to(*o);
-      onode_lru.erase(q);
+      _onode_lru_erase(q);
     }
     void _touch_onode(OnodeRef& o) override;
 
diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc
index 47fb19ee829..2d687ed280d 100644
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -15,6 +15,7 @@
 #include <glob.h>
 #include <stdio.h>
 #include <string.h>
+#include <set>
 #include <iostream>
 #include <time.h>
 #include <sys/mount.h>
@@ -139,6 +140,7 @@ public:
   void doSyntheticTest(
     int num_ops,
     uint64_t max_obj, uint64_t max_wr, uint64_t align);
+  void doOnodeCacheTrimTest();
 };
 
 class StoreTestDeferredSetup : public StoreTest {
@@ -8063,6 +8065,142 @@ TEST_P(StoreTestSpecificAUSize, SpilloverFixed2Test) {
   );
 }
 
+void StoreTest::doOnodeCacheTrimTest() {
+  int r;
+  coll_t cid(spg_t(pg_t(0, 1), shard_id_t(1)));
+  auto ch = store->create_new_collection(cid);
+  {
+    ObjectStore::Transaction t;
+    t.create_collection(cid, 0);
+    cerr << "Creating collection " << cid << std::endl;
+    r = queue_transaction(store, ch, std::move(t));
+    ASSERT_EQ(r, 0);
+  }
+  vector<ghobject_t> all;
+  const size_t max_onodes = 2000;
+  const size_t max_pinned_onodes = 200;
+  const size_t max_cached_onodes = max_pinned_onodes / 2;
+  const PerfCounters* logger = store->get_perf_counters();
+  size_t onodes;
+  {
+    ObjectStore::Transaction t;
+    for (size_t i = 0; i < max_onodes; ++i) {
+      string name("object_");
+      name += stringify(i);
+      ghobject_t hoid(hobject_t(sobject_t(name, CEPH_NOSNAP)),
+		      ghobject_t::NO_GEN, shard_id_t(1));
+      hoid.hobj.pool = 1;
+      all.emplace_back(hoid);
+      t.touch(cid, hoid);
+      if ((i % 100) == 0) {
+        cerr << "Creating object " << hoid << std::endl;
+      }
+    }
+    r = queue_transaction(store, ch, std::move(t));
+    ASSERT_EQ(r, 0);
+  }
+  for (size_t i = 0; i < 5; ++i) {
+    onodes = logger->get(l_bluestore_onodes);
+    if (onodes == max_onodes)
+      break;
+    sleep(1);
+  }
+  ceph_assert(onodes == max_onodes);
+
+  SetVal(g_conf(), "bluestore_debug_max_cached_onodes",
+    stringify(max_cached_onodes).c_str());
+
+  for (size_t i = 0; i < 5; ++i) {
+    cerr << " remaining onodes = "
+	 << logger->get(l_bluestore_onodes)
+	 << std::endl;
+    sleep(1);
+  }
+  onodes = logger->get(l_bluestore_onodes);
+  ceph_assert(onodes == max_cached_onodes);
+
+
+  // revert cache size cap
+  SetVal(g_conf(), "bluestore_debug_max_cached_onodes", "0");
+
+  // pin some onodes
+  vector <ObjectMap::ObjectMapIterator> omap_iterators;
+  for (size_t i = 0; i < max_pinned_onodes; ++i) {
+    omap_iterators.emplace_back(store->get_omap_iterator(ch, all[i]));
+  }
+  // "warm" non-pinned onodes
+  {
+    ObjectStore::Transaction t;
+    for (size_t i = max_pinned_onodes; i < max_onodes; ++i) {
+      t.touch(cid, all[i]);
+    }
+    r = queue_transaction(store, ch, std::move(t));
+    ASSERT_EQ(r, 0);
+  }
+
+  for (size_t i = 0; i < 5; ++i) {
+    onodes = logger->get(l_bluestore_onodes);
+    if (onodes == max_onodes)
+      break;
+    sleep(1);
+  }
+  ceph_assert(onodes == max_onodes);
+
+  SetVal(g_conf(), "bluestore_debug_max_cached_onodes",
+    stringify(max_cached_onodes).c_str());
+
+  for (size_t i = 0; i < 5; ++i) {
+    cerr << " remaining onodes = "
+	 << logger->get(l_bluestore_onodes)
+	 << std::endl;
+    sleep(1);
+  }
+  onodes = logger->get(l_bluestore_onodes);
+  ceph_assert(onodes == max_pinned_onodes);
+
+  // unpin onodes
+  omap_iterators.resize(0);
+
+  for (size_t i = 0; i < 5; ++i) {
+    cerr << " remaining onodes = "
+	 << logger->get(l_bluestore_onodes)
+	 << std::endl;
+    sleep(1);
+  }
+  onodes = logger->get(l_bluestore_onodes);
+  ceph_assert(onodes == max_cached_onodes);
+
+  {
+    ObjectStore::Transaction t;
+    for (size_t i = 0; i < max_onodes; ++i)
+      t.remove(cid, all[i]);
+    t.remove_collection(cid);
+    cerr << "Cleaning" << std::endl;
+    r = queue_transaction(store, ch, std::move(t));
+    ASSERT_EQ(r, 0);
+  }
+}
+
+TEST_P(StoreTestSpecificAUSize, OnodeCacheTrim2QTest) {
+  if (string(GetParam()) != "bluestore")
+    return;
+  SetVal(g_conf(), "bluestore_cache_type", "2q");
+  g_conf().apply_changes(nullptr);
+
+  StartDeferred(65536);
+  doOnodeCacheTrimTest();
+}
+
+TEST_P(StoreTestSpecificAUSize, OnodeCacheTrimLRUTest) {
+  if (string(GetParam()) != "bluestore")
+    return;
+  SetVal(g_conf(), "bluestore_cache_type", "lru");
+  g_conf().apply_changes(nullptr);
+
+  StartDeferred(65536);
+  doOnodeCacheTrimTest();
+}
+
 #endif  // WITH_BLUESTORE
 
 int main(int argc, char **argv) {
-- 
2.47.3