From: Samuel Just Date: Fri, 22 Nov 2024 02:34:12 +0000 (-0800) Subject: crimson: clear obcs whether referenced or not upon replica write X-Git-Tag: v20.0.0~524^2~26 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=7a36f74ceab5773b6ac411f12e9b824123dc6a53;p=ceph.git 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 --- diff --git a/src/common/intrusive_lru.h b/src/common/intrusive_lru.h index 667f73e61b095..f9132773e4d9b 100644 --- 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 + 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(*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 + 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)); } - /// 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 void clear(F &&f) { - evict(0); - assert(unreferenced_list.empty()); - for (auto &i: lru_set) { - std::invoke(f, static_cast(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)); } template diff --git a/src/crimson/osd/object_context.h b/src/crimson/osd/object_context.h index 7ee86ad673be4..febd5d53ec136 100644 --- 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 af8edb8e2bf3a..1410b73fcaad5 100644 --- 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);