From: Igor Fedotov <ifedotov@suse.com>
Date: Sat, 23 Jan 2021 17:33:13 +0000 (+0300)
Subject: os/bluestore: fix a bug causing unexpected Onode's unpinned state.
X-Git-Tag: v15.2.9~8^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=99a48f43af30fbf457d5d0f9f10a7c40d1231c39;p=ceph.git

os/bluestore: fix a bug causing unexpected Onode's unpinned state.

There could be a race for Onodes put() and get() methods:

put()(pinned, nref=3)
  int n = --nref; (nref = 2)
  if (n == 2) {
    ..
    std::lock_guard l(ocs->lock);
    ...
    pinned = pinned && nref > 2; (= false)
    ...                                     get()
    if (r) {                                ++nref; (=3)
      n = --nref; (nref = 2)                return;
    }
    ...
    return

As a result nref = 2, pinned = false which is wrong

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

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 8ebf2992099..2425580ceeb 100644
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -3554,7 +3554,7 @@ BlueStore::BlobRef BlueStore::ExtentMap::split_blob(
 // decremented. And another 'putting' thread on the instance will release it.
 //
 void BlueStore::Onode::get() {
-  if (++nref == 2) {
+  if (++nref >= 2 && !pinned) {
     OnodeCacheShard* ocs = c->get_onode_cache();
     std::lock_guard l(ocs->lock);
     bool was_pinned = pinned;
@@ -3574,15 +3574,11 @@ void BlueStore::Onode::put() {
   if (n == 2) {
     OnodeCacheShard* ocs = c->get_onode_cache();
     std::lock_guard l(ocs->lock);
-    bool was_pinned = pinned;
+    bool need_unpin = 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) {
-      n = --nref;
-    }
-    if (cached && r) {
+                                 // +1 due to pinned state
+    need_unpin = need_unpin && !pinned;
+    if (cached && need_unpin) {
       if (exists) {
         ocs->_unpin(this);
       } else {
@@ -3591,6 +3587,12 @@ void BlueStore::Onode::put() {
         c->onode_map._remove(oid);
       }
     }
+    // additional decrement for newly unpinned instance
+    // should be the last action since Onode can be released
+    // at any point after this decrement
+    if (need_unpin) {
+      n = --nref;
+    }
   }
   if (n == 0) {
     delete this;
@@ -4041,7 +4043,7 @@ void BlueStore::Collection::split_cache(
 
       p = onode_map.onode_map.erase(p);
       dest->onode_map.onode_map[o->oid] = o;
-      if (get_onode_cache() != dest->get_onode_cache()) {
+      if (o->cached) {
         get_onode_cache()->move_pinned(dest->get_onode_cache(), o.get());
       }
       o->c = dest;
diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index 1b6bde9638e..2736e59f2cf 100644
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -1071,7 +1071,7 @@ public:
     bool cached;              ///< Onode is logically in the cache
                               /// (it can be pinned and hence physically out
                               /// of it at the moment though)
-    bool pinned;              ///< Onode is pinned
+    std::atomic_bool pinned;  ///< Onode is pinned
                               /// (or should be pinned when cached)
     ExtentMap extent_map;