crimson: clear obcs whether referenced or not upon replica write

It's always possible for there to be an in-progress replica-read
when the replica processes a repop.  It's rare in our tests because
the read and write submitted by the test client would need to
overlap in time.  This makes the results non-deterministic and
thus a somewhat less sensitive test.  Note, the space of valid
results is well defined -- it would have to be state before or
after any of the outstanding writes.  Any other result or a torn
read would be wrong.  It's probably worth updating RadosModel
to add such a pattern, but we can do that later.

This branch makes this race much more likely and even observable
with the existing RadosModel implementation as it extends the
obc lifetime past the point of returning the result to the client
in order to ensure that it outlives the handle.

Fixes: https://tracker.ceph.com/issues/69013
Signed-off-by: Samuel Just <sjust@redhat.com>

diff --git a/src/common/intrusive_lru.h b/src/common/intrusive_lru.h
index 667f73e61b09549972a882824f469320ab0aa82d..f9132773e4d9b9a97b5eeb0963a7941c312de8a0 100644 (file)
--- a/src/common/intrusive_lru.h
+++ b/src/common/intrusive_lru.h
@@ -178,6 +178,25 @@ class intrusive_lru {
     evict(lru_target_size);
   }
 
+  /// clear [from, to) invoking f upon and invalidating any live references
+  template <typename F>
+  void clear_range(
+    typename lru_set_t::iterator from, typename lru_set_t::iterator to,
+    F &&f) {
+    while (from != to) {
+      if (!(*from).lru) {
+       unreferenced_list.erase(lru_list_t::s_iterator_to(*from));
+       from = lru_set.erase_and_dispose(from, [](auto *p) { delete p; } );
+      } else {
+       std::invoke(f, static_cast<T&>(*from));
+       from->lru = nullptr;
+       assert(from->is_invalidated());
+       from = lru_set.erase_and_dispose(
+         from, [](auto *p) { assert(p->use_count > 0); });
+      }
+    }
+  }
+
 public:
   /**
    * Returns the TRef corresponding to k if it exists or
@@ -200,38 +219,28 @@ public:
     }
   }
 
-  /*
-   * Clears unreferenced elements from the lru set [from, to]
+  /**
+   * clear_range
+   *
+   * Clears elements from the lru set in [from, to] invoking F upon and
+   * invalidating any with outstanding references
    */
-  void clear_range(
-    const K& from,
-    const K& to) {
-      auto from_iter = lru_set.lower_bound(from);
-      auto to_iter = lru_set.upper_bound(to);
-      for (auto i = from_iter; i != to_iter; ) {
-        if (!(*i).lru) {
-          unreferenced_list.erase(lru_list_t::s_iterator_to(*i));
-          i = lru_set.erase_and_dispose(i, [](auto *p)
-            { delete p; } );
-        } else {
-          i++;
-        }
-      }
+  template <typename F>
+  void clear_range(const K& from, const K& to, F &&f) {
+    auto from_iter = lru_set.lower_bound(from);
+    auto to_iter = lru_set.upper_bound(to);
+    clear_range(from_iter, to_iter, std::forward<F>(f));
   }
 
-  /// drop all elements from lru, invoke f on any with outstanding references
+  /**
+   * clear
+   *
+   * Clears all elements from the lru set invoking F upon and
+   * invalidating any with outstanding references
+   */
   template <typename F>
   void clear(F &&f) {
-    evict(0);
-    assert(unreferenced_list.empty());
-    for (auto &i: lru_set) {
-      std::invoke(f, static_cast<T&>(i));
-      i.lru = nullptr;
-      assert(i.is_invalidated());
-    }
-    lru_set.clear_and_dispose([](auto *i){
-      assert(i->use_count > 0); /* don't delete, still has a ref count */
-    });
+    clear_range(lru_set.begin(), lru_set.end(), std::forward<F>(f));
   }
 
   template <class F>

diff --git a/src/crimson/osd/object_context.h b/src/crimson/osd/object_context.h
index 7ee86ad673be4a8e61282f38f381adc154ef2a20..febd5d53ec136345c5c0116689452c2cdfda94e0 100644 (file)
--- a/src/crimson/osd/object_context.h
+++ b/src/crimson/osd/object_context.h
@@ -129,14 +129,14 @@ public:
   }
 
   bool is_valid() const {
-    return !invalidated_by_interval_change;
+    return !invalidated;
   }
 
 private:
   boost::intrusive::list_member_hook<> obc_accessing_hook;
   uint64_t list_link_cnt = 0;
   bool fully_loaded = false;
-  bool invalidated_by_interval_change = false;
+  bool invalidated = false;
 
   friend class ObjectContextRegistry;
   friend class ObjectContextLoader;
@@ -196,12 +196,14 @@ public:
 
   void clear_range(const hobject_t &from,
                    const hobject_t &to) {
-    obc_lru.clear_range(from, to);
+    obc_lru.clear_range(from, to, [](auto &obc) {
+      obc.invalidated = true;
+    });
   }
 
   void invalidate_on_interval_change() {
     obc_lru.clear([](auto &obc) {
-      obc.invalidated_by_interval_change = true;
+      obc.invalidated = true;
     });
   }
 

diff --git a/src/test/common/test_intrusive_lru.cc b/src/test/common/test_intrusive_lru.cc
index af8edb8e2bf3a2b62e95c8f7df66a7672e90ffb5..1410b73fcaad5eb7949f85a0070a0dfd7a4e6114 100644 (file)
--- a/src/test/common/test_intrusive_lru.cc
+++ b/src/test/common/test_intrusive_lru.cc
@@ -177,9 +177,9 @@ TEST(LRU, clear_range) {
   auto [live_ref2, existed2] = cache.add(5, 4);
   ASSERT_FALSE(existed2);
 
-  cache.clear_range(0,4);
+  cache.clear_range(0, 4, [](auto&){});
 
-  // Should not exists (Unreferenced):
+  // Should not exist
   {
     auto [ref, existed] = cache.add(1, 4);
     ASSERT_FALSE(existed);
@@ -192,21 +192,27 @@ TEST(LRU, clear_range) {
     auto [ref, existed] = cache.add(3, 4);
     ASSERT_FALSE(existed);
   }
-  // Should exist (Still being referenced):
   {
     auto [ref, existed] = cache.add(4, 4);
-    ASSERT_TRUE(existed);
+    ASSERT_FALSE(existed);
   }
-  // Should exists (Still being referenced and wasn't removed)
+  ASSERT_TRUE(live_ref1->is_invalidated());
+  // Should exist, wasn't removed)
   {
     auto [ref, existed] = cache.add(5, 4);
     ASSERT_TRUE(existed);
   }
-  // Test out of bound deletion:
+  ASSERT_FALSE(live_ref2->is_invalidated());
+  // Test clear_range with right bound past last entry
+  cache.clear_range(3, 8, [](auto&){});
+  ASSERT_TRUE(live_ref2->is_invalidated());
   {
-    cache.clear_range(3,8);
     auto [ref, existed] = cache.add(4, 4);
-    ASSERT_TRUE(existed);
+    ASSERT_FALSE(existed);
+  }
+  {
+    auto [ref, existed] = cache.add(5, 4);
+    ASSERT_FALSE(existed);
   }
   {
     auto [ref, existed] = cache.add(3, 4);