From: Sage Weil <sage@redhat.com>
Date: Sun, 27 Oct 2019 14:59:10 +0000 (-0500)
Subject: Revert "Merge pull request #30964 from markhpc/wip-bs-cache-trim-pinned"
X-Git-Tag: v15.1.0~1130^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=f03395e5a60968fc0142644bdd3e81e8277cf71a;p=ceph.git

Revert "Merge pull request #30964 from markhpc/wip-bs-cache-trim-pinned"

This reverts commit 304c37f52165363cad4aaf31386d62ea5a573425, reversing
changes made to 7b4f9a083f0c8cc015f7b0cc12a3bc88db632610.

This causes some bluestore test failures due to an ENOENT right after a
new object is created.  The simplest reproducer is the
ObjectStore/StoreTest.FiemapEmpty/2 test.

Fixes: https://tracker.ceph.com/issues/42495
Signed-off-by: Sage Weil <sage@redhat.com>
---

diff --git a/src/common/options.cc b/src/common/options.cc
index 632753c92d155..aea223ca8d760 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -2762,11 +2762,6 @@ std::vector<Option> get_global_options() {
     .set_default(true)
     .set_description(""),
 
-    Option("osd_num_cache_shards", Option::TYPE_SIZE, Option::LEVEL_ADVANCED)
-    .set_default(32)
-    .set_flag(Option::FLAG_STARTUP)
-    .set_description("The number of cache shards to use in the object store."),
-
     Option("osd_op_num_threads_per_shard", Option::TYPE_INT, Option::LEVEL_ADVANCED)
     .set_default(0)
     .set_flag(Option::FLAG_STARTUP)
diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 89c41273016d8..c2b77bb47cfbe 100644
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -841,81 +841,58 @@ struct LruOnodeCacheShard : public BlueStore::OnodeCacheShard {
       BlueStore::Onode,
       boost::intrusive::list_member_hook<>,
       &BlueStore::Onode::lru_item> > list_t;
-  typedef boost::intrusive::list<
-    BlueStore::Onode,
-    boost::intrusive::member_hook<
-      BlueStore::Onode,
-      boost::intrusive::list_member_hook<>,
-      &BlueStore::Onode::pin_item> > pin_list_t;
-
   list_t lru;
-  pin_list_t pin_list;
 
   explicit LruOnodeCacheShard(CephContext *cct) : BlueStore::OnodeCacheShard(cct) {}
 
   void _add(BlueStore::OnodeRef& o, int level) override
   {
     (level > 0) ? lru.push_front(*o) : lru.push_back(*o);
-    o->s = this;
     num = lru.size();
   }
   void _rm(BlueStore::OnodeRef& o) override
   {
-    o->s = nullptr;
-    if (o->pinned) {
-      o->pinned = false;
-      pin_list.erase(pin_list.iterator_to(*o));
-    } else {
-      lru.erase(lru.iterator_to(*o));
-    }
+    lru.erase(lru.iterator_to(*o));
     num = lru.size();
-    num_pinned = pin_list.size();
   }
   void _touch(BlueStore::OnodeRef& o) override
   {
-    if (o->pinned) {
-      return;
-    }
     lru.erase(lru.iterator_to(*o));
     lru.push_front(*o);
     num = lru.size();
   }
-  void _pin(BlueStore::Onode& o) override
-  {
-    if (o.pinned == true) {
-      return;
-    }
-    lru.erase(lru.iterator_to(o));
-    pin_list.push_front(o);
-    o.pinned = true;
-    num = lru.size();
-    num_pinned = pin_list.size();
-    dout(30) << __func__ << " " << o.oid << " pinned" << dendl;
-
-  } 
-  void _unpin(BlueStore::Onode& o) override
-  {
-    if (o.pinned == false) {
-      return;
-    }
-    pin_list.erase(pin_list.iterator_to(o));
-    lru.push_front(o);
-    o.pinned = false;
-    num = lru.size();
-    num_pinned = pin_list.size();
-    dout(30) << __func__ << " " << o.oid << " unpinned" << dendl;
-  }
-  void _trim_to(uint64_t new_size) override
+  void _trim_to(uint64_t max) override
   {
-    if (new_size >= lru.size()) {
+    if (max >= lru.size()) {
       return; // don't even try
     } 
-    uint64_t n = lru.size() - new_size;
+    uint64_t n = lru.size() - max;
+
     auto p = lru.end();
     ceph_assert(p != lru.begin());
     --p;
+    int skipped = 0;
+    int max_skipped = g_conf()->bluestore_cache_trim_max_skip_pinned;
     while (n > 0) {
       BlueStore::Onode *o = &*p;
+      int refs = o->nref.load();
+      if (refs > 1) {
+        dout(20) << __func__ << "  " << o->oid << " has " << refs
+                 << " refs, skipping" << dendl;
+        if (++skipped >= max_skipped) {
+          dout(20) << __func__ << " maximum skip pinned reached; stopping with "
+                   << n << " left to trim" << dendl;
+          break;
+        }
+
+        if (p == lru.begin()) {
+          break;
+        } else {
+          p--;
+          n--;
+          continue;
+        }
+      }
       dout(30) << __func__ << "  rm " << o->oid << dendl;
       if (p != lru.begin()) {
         lru.erase(p--);
@@ -923,7 +900,6 @@ struct LruOnodeCacheShard : public BlueStore::OnodeCacheShard {
         lru.erase(p);
         ceph_assert(n == 1);
       }
-      o->s = nullptr;
       o->get();  // paranoia
       o->c->onode_map.remove(o->oid);
       o->put();
@@ -931,10 +907,9 @@ struct LruOnodeCacheShard : public BlueStore::OnodeCacheShard {
     }
     num = lru.size();
   }
-  void add_stats(uint64_t *onodes, uint64_t *pinned_onodes) override
+  void add_stats(uint64_t *onodes) override
   {
-    *onodes += num + num_pinned;
-    *pinned_onodes += num_pinned;
+    *onodes += num;
   }
 };
 
@@ -4578,8 +4553,6 @@ void BlueStore::_init_logger()
 
   b.add_u64(l_bluestore_onodes, "bluestore_onodes",
 	    "Number of onodes in cache");
-  b.add_u64(l_bluestore_pinned_onodes, "bluestore_pinned_onodes",
-            "Number of pinned onodes in cache");
   b.add_u64_counter(l_bluestore_onode_hits, "bluestore_onode_hits",
 		    "Sum for onode-lookups hit in the cache");
   b.add_u64_counter(l_bluestore_onode_misses, "bluestore_onode_misses",
@@ -9200,20 +9173,18 @@ void BlueStore::_reap_collections()
 void BlueStore::_update_cache_logger()
 {
   uint64_t num_onodes = 0;
-  uint64_t num_pinned_onodes = 0;
   uint64_t num_extents = 0;
   uint64_t num_blobs = 0;
   uint64_t num_buffers = 0;
   uint64_t num_buffer_bytes = 0;
   for (auto c : onode_cache_shards) {
-    c->add_stats(&num_onodes, &num_pinned_onodes);
+    c->add_stats(&num_onodes);
   }
   for (auto c : buffer_cache_shards) {
     c->add_stats(&num_extents, &num_blobs,
                  &num_buffers, &num_buffer_bytes);
   }
   logger->set(l_bluestore_onodes, num_onodes);
-  logger->set(l_bluestore_pinned_onodes, num_pinned_onodes);
   logger->set(l_bluestore_extents, num_extents);
   logger->set(l_bluestore_blobs, num_blobs);
   logger->set(l_bluestore_buffers, num_buffers);
diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index ff4596a95d84d..9baeca0218829 100644
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -103,7 +103,6 @@ enum {
   l_bluestore_compressed_allocated,
   l_bluestore_compressed_original,
   l_bluestore_onodes,
-  l_bluestore_pinned_onodes,
   l_bluestore_onode_hits,
   l_bluestore_onode_misses,
   l_bluestore_onode_shard_hits,
@@ -1050,22 +1049,20 @@ public:
   };
 
   struct OnodeSpace;
-  struct OnodeCacheShard;
+
   /// an in-memory object
   struct Onode {
     MEMPOOL_CLASS_HELPERS();
-    // Not persisted and updated on cache insertion/removal
-    OnodeCacheShard *s;
-    bool pinned = false; // Only to be used by the onode cache shard
 
     std::atomic_int nref;  ///< reference count
     Collection *c;
+
     ghobject_t oid;
 
     /// key under PREFIX_OBJ where we are stored
     mempool::bluestore_cache_other::string key;
 
-    boost::intrusive::list_member_hook<> lru_item, pin_item;
+    boost::intrusive::list_member_hook<> lru_item;
 
     bluestore_onode_t onode;  ///< metadata stored as value in kv store
     bool exists;              ///< true if object logically exists
@@ -1082,8 +1079,7 @@ public:
 
     Onode(Collection *c, const ghobject_t& o,
 	  const mempool::bluestore_cache_other::string& k)
-      : s(nullptr),
-        nref(0),
+      : nref(0),
 	c(c),
 	oid(o),
 	key(k),
@@ -1092,8 +1088,7 @@ public:
     }
     Onode(Collection* c, const ghobject_t& o,
       const string& k)
-      : s(nullptr),
-      nref(0),
+      : nref(0),
       c(c),
       oid(o),
       key(k),
@@ -1102,8 +1097,7 @@ public:
     }
     Onode(Collection* c, const ghobject_t& o,
       const char* k)
-      : s(nullptr),
-      nref(0),
+      : nref(0),
       c(c),
       oid(o),
       key(k),
@@ -1121,18 +1115,11 @@ public:
 
     void flush();
     void get() {
-      if (++nref == 2 && s != nullptr) {
-        s->pin(*this);
-      }
+      ++nref;
     }
     void put() {
-      int n = --nref;
-      if (n == 1 && s != nullptr) {
-        s->unpin(*this);
-      }
-      if (n == 0) {
+      if (--nref == 0)
 	delete this;
-      }
     }
 
     const string& get_omap_prefix();
@@ -1167,7 +1154,7 @@ public:
       return num;
     }
 
-    virtual void _trim_to(uint64_t new_size) = 0;
+    virtual void _trim_to(uint64_t max) = 0;
     void _trim() {
       if (cct->_conf->objectstore_blackhole) {
 	// do not trim if we are throwing away IOs a layer down
@@ -1175,7 +1162,6 @@ public:
       }
       _trim_to(max);
     }
-
     void trim() {
       std::lock_guard l(lock);
       _trim();    
@@ -1196,8 +1182,6 @@ public:
 
   /// A Generic onode Cache Shard
   struct OnodeCacheShard : public CacheShard {
-    std::atomic<uint64_t> num_pinned = {0};
-
     std::array<std::pair<ghobject_t, mono_clock::time_point>, 64> dumped_onodes;
   public:
     OnodeCacheShard(CephContext* cct) : CacheShard(cct) {}
@@ -1206,20 +1190,8 @@ public:
     virtual void _add(OnodeRef& o, int level) = 0;
     virtual void _rm(OnodeRef& o) = 0;
     virtual void _touch(OnodeRef& o) = 0;
-    virtual void _pin(Onode& o) = 0;
-    virtual void _unpin(Onode& o) = 0;
-
-    void pin(Onode& o) {
-      std::lock_guard l(lock);
-      _pin(o);
-    }
-
-    void unpin(Onode& o) {
-      std::lock_guard l(lock);
-      _unpin(o);
-    }
+    virtual void add_stats(uint64_t *onodes) = 0;
 
-    virtual void add_stats(uint64_t *onodes, uint64_t *pinned_onodes) = 0;
     bool empty() {
       return _get_num() == 0;
     }
diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index 13555c137ebbb..bba59d7073b25 100644
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -3168,11 +3168,6 @@ int OSD::enable_disable_fuse(bool stop)
   return 0;
 }
 
-size_t OSD::get_num_cache_shards()
-{
-  return cct->_conf.get_val<Option::size_t>("osd_num_cache_shards");
-}
-
 int OSD::get_num_op_shards()
 {
   if (cct->_conf->osd_op_num_shards)
@@ -3266,7 +3261,7 @@ int OSD::init()
   dout(2) << "journal " << journal_path << dendl;
   ceph_assert(store);  // call pre_init() first!
 
-  store->set_cache_shards(get_num_cache_shards());
+  store->set_cache_shards(get_num_op_shards());
 
   int r = store->mount();
   if (r < 0) {
diff --git a/src/osd/OSD.h b/src/osd/OSD.h
index 7fceecfbeb14b..7713c208f8a6e 100644
--- a/src/osd/OSD.h
+++ b/src/osd/OSD.h
@@ -2033,7 +2033,6 @@ private:
 
   int init_op_flags(OpRequestRef& op);
 
-  size_t get_num_cache_shards();
   int get_num_op_shards();
   int get_num_op_threads();