os/bluestore: fix pinned/cached setting in Onode::put/get

This to be squashed with HEAD~2 later, left standalone to ease
reviewing for now.

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
(cherry picked from commit 0ef5ebcd5df38dab5cf0498ca35ed061a47b07ba)

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 9ee3365286537c7263f077e62fff112d0cd2795a..b44df72ba405627b803bb4610d66fabd3bef5dfb 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -890,7 +890,8 @@ struct LruOnodeCacheShard : public BlueStore::OnodeCacheShard {
     --p;
     while (n > 0) {
       BlueStore::Onode *o = &*p;
-      dout(30) << __func__ << "  rm " << o->oid << dendl;
+      dout(30) << __func__ << "  rm " << o->oid << " "
+               << o->nref << " " << o->cached << " " << o->pinned << dendl;
       if (p != lru.begin()) {
         lru.erase(p--);
       } else {
@@ -1633,6 +1634,9 @@ BlueStore::OnodeRef BlueStore::OnodeSpace::lookup(const ghobject_t& oid)
       ldout(cache->cct, 30) << __func__ << " " << oid << " miss" << dendl;
     } else {
       ldout(cache->cct, 30) << __func__ << " " << oid << " hit " << p->second
+                            << " " << p->second->nref
+                            << " " << p->second->cached
+                            << " " << p->second->pinned
                            << dendl;
       // This will pin onode and implicitly touch the cache when Onode
       // eventually will become unpinned
@@ -1721,7 +1725,11 @@ template <int LogLevelV = 30>
 void BlueStore::OnodeSpace::dump(CephContext *cct)
 {
   for (auto& i : onode_map) {
-    ldout(cct, LogLevelV) << i.first << " : " << i.second << dendl;
+    ldout(cct, LogLevelV) << i.first << " : " << i.second
+      << " " << i.second->nref
+      << " " << i.second->cached
+      << " " << i.second->pinned
+      << dendl;
   }
 }
 
@@ -3261,24 +3269,43 @@ BlueStore::BlobRef BlueStore::ExtentMap::split_blob(
 #undef dout_prefix
 #define dout_prefix *_dout << "bluestore.onode(" << this << ")." << __func__ << " "
 
+//
+// A tricky thing about Onode's ref counter is that we do an additional
+// increment when newly pinned instance is detected. And -1 on unpin.
+// This prevents from a conflict with a delete call (when nref == 0).
+// The latter might happen while the thread is in unpin() function
+// (and e.g. waiting for lock acquisition) since nref is already
+// decremented. And another 'putting' thread on the instance will release it.
+//
 void BlueStore::Onode::get() {
   if (++nref == 2) {
     c->get_onode_cache()->pin(this, [&]() {
-        bool r = cached && !pinned;
-        pinned = true;
-        return r;
+        bool was_pinned = pinned;
+        pinned = nref >= 2;
+        // additional increment for newly pinned instance
+        bool r = !was_pinned && pinned;
+        if (r) {
+          ++nref;
+        }
+        return cached && r;
       });
   }
 }
 void BlueStore::Onode::put() {
-  int n = --nref;
-  if (n == 1) {
+  if (--nref == 2) {
     c->get_onode_cache()->unpin(this, [&]() {
-        bool r = cached && pinned;
-        pinned = false;
-        return r;
+        bool was_pinned = pinned;
+        pinned = pinned && nref > 2; // intentionally use > not >= as we have
+                                     // +1 due to pinned state
+        bool r = was_pinned && !pinned;
+        // additional decrement for newly unpinned instance
+        if (r) {
+          --nref;
+        }
+        return cached && r;
       });
-  } else if (n == 0) {
+  }
+  if (nref == 0) {
     delete this;
   }
 }