From dbedb5b909bbd9bad7af040d73e64f5ca43ce0c8 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 13 May 2023 00:30:52 +0000 Subject: [PATCH] Do not populate StoreEntry basics from Transients (#1343) client_side_reply.cc:1078: "http->storeEntry()->objectLen() >= 0" inside clientReplyContext::storeOKTransferDone() Since commit 24c9378, Transients are not supposed to maintain "basic" cache entry information (i.e. StoreMapAnchor::Basics a.k.a. StoreEntry STORE_META_STD fields). Using often-at-their-defaults Transient basics may unexpectedly erase valuable StoreEntry information obtained from one of the caches (e.g., swap_file_sz) and lead to assertions. Consequently, do not populate unused Transients entry basics either. Eventually, we may find a good way to parameterize StoreMapAnchor to avoid keeping those unused fields in Transients StoreMap. --- src/MemObject.h | 14 ++++++++++++++ src/Transients.cc | 28 ++++++---------------------- src/Transients.h | 1 - src/ipc/StoreMap.cc | 4 ++-- src/ipc/StoreMap.h | 2 +- 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/MemObject.h b/src/MemObject.h index adc786e820..0a7be72046 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -169,6 +169,20 @@ public: class XitTable { public: + /// associate our StoreEntry with a Transients entry at the given index + void open(const int32_t anIndex, const Io anIo) + { + index = anIndex; + io = anIo; + } + + /// stop associating our StoreEntry with a Transients entry + void close() + { + index = -1; + io = Store::ioDone; + } + int32_t index = -1; ///< entry position inside the in-transit table Io io = ioUndecided; ///< current I/O state }; diff --git a/src/Transients.cc b/src/Transients.cc index 16575606a3..fbcf832c1d 100644 --- a/src/Transients.cc +++ b/src/Transients.cc @@ -166,7 +166,7 @@ Transients::get(const cache_key *key) StoreEntry *e = new StoreEntry(); e->createMemObject(); - anchorEntry(*e, index, *anchor); + e->mem_obj->xitTable.open(index, Store::ioReading); // keep read lock to receive updates from others return e; @@ -234,11 +234,9 @@ Transients::addWriterEntry(StoreEntry &e, const cache_key *key) // set ASAP in hope to unlock the slot if something throws // and to provide index to such methods as hasWriter() - auto &xitTable = e.mem_obj->xitTable; - xitTable.index = index; - xitTable.io = Store::ioWriting; + e.mem_obj->xitTable.open(index, Store::ioWriting); - anchor->set(e, key); + anchor->setKey(key); // allow reading and receive remote DELETE events, but do not switch to // the reading lock because transientReaders() callers want true readers map->startAppending(index); @@ -250,27 +248,14 @@ void Transients::addReaderEntry(StoreEntry &e, const cache_key *key) { sfileno index = 0; - const auto anchor = map->openOrCreateForReading(key, index, e); + const auto anchor = map->openOrCreateForReading(key, index); if (!anchor) throw TextException("reader collision", Here()); - anchorEntry(e, index, *anchor); + e.mem_obj->xitTable.open(index, Store::ioReading); // keep the entry locked (for reading) to receive remote DELETE events } -/// fills (recently created) StoreEntry with information currently in Transients -void -Transients::anchorEntry(StoreEntry &e, const sfileno index, const Ipc::StoreMapAnchor &anchor) -{ - // set ASAP in hope to unlock the slot if something throws - // and to provide index to such methods as hasWriter() - auto &xitTable = e.mem_obj->xitTable; - xitTable.index = index; - xitTable.io = Store::ioReading; - - anchor.exportInto(e); -} - bool Transients::hasWriter(const StoreEntry &e) { @@ -362,8 +347,7 @@ Transients::disconnect(StoreEntry &entry) map->closeForReadingAndFreeIdle(xitTable.index); } locals->at(xitTable.index) = nullptr; - xitTable.index = -1; - xitTable.io = Store::ioDone; + xitTable.close(); } } diff --git a/src/Transients.h b/src/Transients.h index e05fdb73bd..9ba325e765 100644 --- a/src/Transients.h +++ b/src/Transients.h @@ -94,7 +94,6 @@ protected: void addEntry(StoreEntry*, const cache_key *, const Store::IoStatus); void addWriterEntry(StoreEntry &, const cache_key *); void addReaderEntry(StoreEntry &, const cache_key *); - void anchorEntry(StoreEntry &, const sfileno, const Ipc::StoreMapAnchor &); // Ipc::StoreMapCleaner API void noteFreeMapSlice(const Ipc::StoreMapSliceId sliceId) override; diff --git a/src/ipc/StoreMap.cc b/src/ipc/StoreMap.cc index 89d80c8a01..50bcbb9960 100644 --- a/src/ipc/StoreMap.cc +++ b/src/ipc/StoreMap.cc @@ -100,7 +100,7 @@ Ipc::StoreMap::forgetWritingEntry(sfileno fileno) } const Ipc::StoreMap::Anchor * -Ipc::StoreMap::openOrCreateForReading(const cache_key *const key, sfileno &fileno, const StoreEntry &entry) +Ipc::StoreMap::openOrCreateForReading(const cache_key *const key, sfileno &fileno) { debugs(54, 5, "opening/creating entry with key " << storeKeyText(key) << " for reading " << path); @@ -115,7 +115,7 @@ Ipc::StoreMap::openOrCreateForReading(const cache_key *const key, sfileno &filen // the competing openOrCreateForReading() workers race to create a new entry idx = fileNoByKey(key); if (auto anchor = openForWritingAt(idx)) { - anchor->set(entry, key); + anchor->setKey(key); anchor->lock.switchExclusiveToShared(); // race ended assert(anchor->complete()); diff --git a/src/ipc/StoreMap.h b/src/ipc/StoreMap.h index 255c713ec5..616e3e5a2b 100644 --- a/src/ipc/StoreMap.h +++ b/src/ipc/StoreMap.h @@ -314,7 +314,7 @@ public: void closeForReadingAndFreeIdle(const sfileno fileno); /// openForReading() but creates a new entry if there is no old one - const Anchor *openOrCreateForReading(const cache_key *, sfileno &, const StoreEntry &); + const Anchor *openOrCreateForReading(const cache_key *, sfileno &); /// writeable slice within an entry chain created by openForWriting() Slice &writeableSlice(const AnchorId anchorId, const SliceId sliceId); -- 2.39.2