From: Eduard Bagdasaryan Date: Wed, 15 Jan 2020 15:57:14 +0000 (+0000) Subject: Do not stall if xactions overwrite a recently active cache entry (#516) X-Git-Tag: 4.15-20210522-snapshot~174 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d1d3b4dce2e25663aa0565dc630b2c97aa199065;p=thirdparty%2Fsquid.git Do not stall if xactions overwrite a recently active cache entry (#516) After the last transaction that cached or read the reply R1 ended, its Transients entry T1 was not freed. Subsequent requests (with different public keys) could occupy the same shared memory and rock slots (purging unlocked R1), preventing Squid from attaching a T1-derived StoreEntry to the cache. Another request for R1 would receive T1 and stall because its worker W1 kept waiting for a notification from another worker W2, incorrectly assuming that W2 exists and is going to fetch R1 for W1. That request was aborted after a timeout. A Transients entry represents active transaction(s). Broadcasts stop when there are no transactions to inform. We must remove idle (i.e. unlocked) Transients entries to avoid feeding new transactions with stale info. We now do that when unlocking a Transients entry and also double check that a found unattached Transients entry has a writer. --- diff --git a/src/Transients.cc b/src/Transients.cc index 5e434b2a54..652cdc1dd4 100644 --- a/src/Transients.cc +++ b/src/Transients.cc @@ -157,7 +157,7 @@ Transients::get(const cache_key *key) if (StoreEntry *oldE = locals->at(index)) { debugs(20, 3, "not joining private " << *oldE); assert(EBIT_TEST(oldE->flags, KEY_PRIVATE)); - map->closeForReading(index); + map->closeForReadingAndFreeIdle(index); return nullptr; } @@ -251,6 +251,14 @@ Transients::addEntry(StoreEntry *e, const cache_key *key, const Store::IoStatus } } +bool +Transients::hasWriter(const StoreEntry &e) +{ + if (!e.hasTransients()) + return false; + return map->peekAtWriter(e.mem_obj->xitTable.index); +} + void Transients::noteFreeMapSlice(const Ipc::StoreMapSliceId) { @@ -325,7 +333,7 @@ Transients::disconnect(StoreEntry &entry) map->abortWriting(xitTable.index); } else { assert(isReader(entry)); - map->closeForReading(xitTable.index); + map->closeForReadingAndFreeIdle(xitTable.index); } locals->at(xitTable.index) = nullptr; xitTable.index = -1; diff --git a/src/Transients.h b/src/Transients.h index b248991424..8d2c0280ea 100644 --- a/src/Transients.h +++ b/src/Transients.h @@ -85,6 +85,8 @@ public: bool isReader(const StoreEntry &) const; /// whether the entry is in "writing to Transients" I/O state bool isWriter(const StoreEntry &) const; + /// whether we or somebody else is in the "writing to Transients" I/O state + bool hasWriter(const StoreEntry &); static int64_t EntryLimit(); diff --git a/src/ipc/ReadWriteLock.cc b/src/ipc/ReadWriteLock.cc index 49f2d281d4..dbeabad69c 100644 --- a/src/ipc/ReadWriteLock.cc +++ b/src/ipc/ReadWriteLock.cc @@ -91,6 +91,26 @@ Ipc::ReadWriteLock::switchExclusiveToShared() unlockExclusive(); } +bool +Ipc::ReadWriteLock::unlockSharedAndSwitchToExclusive() +{ + assert(readers > 0); + if (!writeLevel++) { // we are the first writer + lock "new" readers out + assert(!appending); + unlockShared(); + if (!readers) { + writing = true; + return true; + } + // somebody is still reading: fall through + } else { + // somebody is still writing: just stop reading + unlockShared(); + } + --writeLevel; + return false; +} + void Ipc::ReadWriteLock::startAppending() { diff --git a/src/ipc/ReadWriteLock.h b/src/ipc/ReadWriteLock.h index 753552e817..75cddee3e8 100644 --- a/src/ipc/ReadWriteLock.h +++ b/src/ipc/ReadWriteLock.h @@ -35,6 +35,9 @@ public: void unlockExclusive(); ///< undo successful exclusiveLock() void unlockHeaders(); ///< undo successful lockHeaders() void switchExclusiveToShared(); ///< stop writing, start reading + /// same as unlockShared() but also attempts to get a writer lock beforehand + /// \returns whether the writer lock was acquired + bool unlockSharedAndSwitchToExclusive(); void startAppending(); ///< writer keeps its lock but also allows reading diff --git a/src/ipc/StoreMap.cc b/src/ipc/StoreMap.cc index f3feb1d633..184ece5510 100644 --- a/src/ipc/StoreMap.cc +++ b/src/ipc/StoreMap.cc @@ -247,10 +247,18 @@ Ipc::StoreMap::peekAtReader(const sfileno fileno) const const Anchor &s = anchorAt(fileno); if (s.reading()) return &s; // immediate access by lock holder so no locking + assert(s.writing()); // must be locked for reading or writing + return nullptr; +} + +const Ipc::StoreMap::Anchor * +Ipc::StoreMap::peekAtWriter(const sfileno fileno) const +{ + const Anchor &s = anchorAt(fileno); if (s.writing()) - return NULL; // the caller is not a read lock holder - assert(false); // must be locked for reading or writing - return NULL; + return &s; // immediate access by lock holder so no locking + assert(s.reading()); // must be locked for reading or writing + return nullptr; } const Ipc::StoreMap::Anchor & @@ -442,6 +450,23 @@ Ipc::StoreMap::closeForReading(const sfileno fileno) debugs(54, 5, "closed entry " << fileno << " for reading " << path); } +void +Ipc::StoreMap::closeForReadingAndFreeIdle(const sfileno fileno) +{ + auto &s = anchorAt(fileno); + assert(s.reading()); + + if (!s.lock.unlockSharedAndSwitchToExclusive()) { + debugs(54, 5, "closed entry " << fileno << " for reading " << path); + return; + } + + assert(s.writing()); + assert(!s.reading()); + freeChain(fileno, s, false); + debugs(54, 5, "closed idle entry " << fileno << " for reading " << path); +} + bool Ipc::StoreMap::openForUpdating(Update &update, const sfileno fileNoHint) { diff --git a/src/ipc/StoreMap.h b/src/ipc/StoreMap.h index 402165d7b7..c4224f2842 100644 --- a/src/ipc/StoreMap.h +++ b/src/ipc/StoreMap.h @@ -259,10 +259,16 @@ public: /// undoes partial update, unlocks, and cleans up void abortUpdating(Update &update); - /// only works on locked entries; returns nil unless the slice is readable + /// the caller must hold a lock on the entry + /// \returns nullptr unless the slice is readable const Anchor *peekAtReader(const sfileno fileno) const; - /// only works on locked entries; returns the corresponding Anchor + /// the caller must hold a lock on the entry + /// \returns nullptr unless the slice is writeable + const Anchor *peekAtWriter(const sfileno fileno) const; + + /// the caller must hold a lock on the entry + /// \returns the corresponding Anchor const Anchor &peekAtEntry(const sfileno fileno) const; /// free the entry if possible or mark it as waiting to be freed if not @@ -285,6 +291,8 @@ public: const Anchor *openForReadingAt(const sfileno fileno); /// closes open entry after reading, decrements read level void closeForReading(const sfileno fileno); + /// same as closeForReading() but also frees the entry if it is unlocked + void closeForReadingAndFreeIdle(const sfileno fileno); /// writeable slice within an entry chain created by openForWriting() Slice &writeableSlice(const AnchorId anchorId, const SliceId sliceId); diff --git a/src/store/Controller.cc b/src/store/Controller.cc index c43c9eca15..ef18b8a85e 100644 --- a/src/store/Controller.cc +++ b/src/store/Controller.cc @@ -334,7 +334,7 @@ Store::Controller::find(const cache_key *key) return entry; } catch (const std::exception &ex) { debugs(20, 2, "failed with " << *entry << ": " << ex.what()); - entry->release(true); + entry->release(); // fall through } } @@ -355,6 +355,18 @@ Store::Controller::allowSharing(StoreEntry &entry, const cache_key *key) const bool found = anchorToCache(entry, inSync); if (found && !inSync) throw TexcHere("cannot sync"); + if (!found) { + // !found should imply hittingRequiresCollapsing() regardless of writer presence + if (!entry.hittingRequiresCollapsing()) { + debugs(20, DBG_IMPORTANT, "BUG: missing ENTRY_REQUIRES_COLLAPSING for " << entry); + throw TextException("transients entry missing ENTRY_REQUIRES_COLLAPSING", Here()); + } + + if (!transients->hasWriter(entry)) { + // prevent others from falling into the same trap + throw TextException("unattached transients entry missing writer", Here()); + } + } } }