From 7fad8b94a7509d052e4f6f773fe718b18964c282 Mon Sep 17 00:00:00 2001 From: Alex Ainscow Date: Tue, 22 Apr 2025 13:41:19 +0100 Subject: [PATCH] osd: Fix EC cache invalidation bug With optimised EC, there were two bugs with cache invalidation: 1. If two invalidates were in the queue, its possible the second invalidate might be cleared by the first. 2. Reads were being requested if size was being reduced. Also, added a few debug improvements and some new asserts. Signed-off-by: Alex Ainscow --- src/osd/ECCommon.h | 2 +- src/osd/ECExtentCache.cc | 42 +++++++++++++++++++++++----------------- src/osd/ECExtentCache.h | 2 +- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/osd/ECCommon.h b/src/osd/ECCommon.h index d27cffabe113c..2771820b69427 100644 --- a/src/osd/ECCommon.h +++ b/src/osd/ECCommon.h @@ -532,7 +532,7 @@ struct ECCommon { << " temp_cleared=" << temp_cleared << " remote_read_result=" << remote_shard_extent_map << " pending_commits=" << pending_commits - << " plan.to_read=" << plan + << " plans=" << plan << ")"; } }; diff --git a/src/osd/ECExtentCache.cc b/src/osd/ECExtentCache.cc index 2f8586ab6c412..80b4a4a714d25 100644 --- a/src/osd/ECExtentCache.cc +++ b/src/osd/ECExtentCache.cc @@ -11,17 +11,12 @@ using namespace std; using namespace ECUtil; void ECExtentCache::Object::request(OpRef &op) { - /* After a cache invalidation, we allow through a single cache-invalidating - * IO. + /* Record that this object is invalidating cache, to avoid any further + * read attempts (which will be discarded). */ if (op->invalidates_cache) { - if (cache_invalidated) { - op->invalidates_cache = false; - } else { - cache_invalidate_expected = true; - } + cache_invalidate_expected = true; } - cache_invalidated = false; extent_set eset = op->get_pin_eset(line_size); @@ -43,6 +38,11 @@ void ECExtentCache::Object::request(OpRef &op) { bool read_required = false; + // If this op previously invalidate cache, the cache had better be empty. + if (op->did_invalidate_cache) { + ceph_assert(do_not_read.empty()); + } + /* Deal with reads if there are any. * If any cache invalidation ops have been added, there is no point adding any * reads as they are all going to be thrown away before any of the @@ -87,15 +87,16 @@ void ECExtentCache::Object::request(OpRef &op) { pg.sinfo.ro_size_to_read_mask(projected_size, read_mask); obj_hole.subtract(read_mask); do_not_read.insert(obj_hole); - } else if (op->projected_size < projected_size) { - // Invalidate the object's cache when we see any object reduce in size. - op->invalidates_cache = true; } projected_size = op->projected_size; - if (read_required) send_reads(); - else op->read_done = true; + if (read_required) { + send_reads(); + } + else { + op->read_done = true; + } } void ECExtentCache::Object::send_reads() { @@ -183,7 +184,7 @@ void ECExtentCache::Object::invalidate(const OpRef &invalidating_op) { line->size = 0; } - /* Remove all entries from the LRU */ + // Remove all entries from the LRU pg.lru.remove_object(oid); ceph_assert(!reading); @@ -198,13 +199,13 @@ void ECExtentCache::Object::invalidate(const OpRef &invalidating_op) { */ projected_size = invalidating_op->projected_size; - // Cache can now be replayed and invalidate teh cache! + // Invalidate cache has been honoured, so no need to repeat. invalidating_op->invalidates_cache = false; + invalidating_op->did_invalidate_cache = true; - cache_invalidated = true; cache_invalidate_expected = false; - /* We now need to reply all outstanding ops, so as to regenerate the read */ + // We now need to reply all outstanding ops to regenerate the read for (auto &op : pg.waiting_ops) { if (op->object.oid == oid) { op->read_done = false; @@ -330,6 +331,10 @@ void ECExtentCache::on_change2() const { void ECExtentCache::execute(list &op_list) { for (auto &op : op_list) { + if (op->projected_size < op->object.projected_size) { + // Invalidate the object's cache when we see any object reduce in size. + op->invalidates_cache = true; + } op->object.request(op); } waiting_ops.insert(waiting_ops.end(), op_list.begin(), op_list.end()); @@ -348,7 +353,8 @@ list::iterator ECExtentCache::LRU::erase( update_mempool(-1, 0 - size_change); } size -= size_change; - map.erase(*it); + size_t removed = map.erase(*it); + ceph_assert(removed == 1); return lru.erase(it); } diff --git a/src/osd/ECExtentCache.h b/src/osd/ECExtentCache.h index 1b82cdbbee48d..b2eafe59ff7f1 100644 --- a/src/osd/ECExtentCache.h +++ b/src/osd/ECExtentCache.h @@ -155,6 +155,7 @@ class ECExtentCache { ECUtil::shard_extent_map_t result; bool complete = false; bool invalidates_cache = false; + bool did_invalidate_cache = false; bool reading = false; bool read_done = false; uint64_t projected_size = 0; @@ -225,7 +226,6 @@ private: uint64_t projected_size = 0; uint64_t line_size = 0; bool reading = false; - bool cache_invalidated = false; bool cache_invalidate_expected = false; void request(OpRef &op); -- 2.39.5