]> git.ipfire.org Git - thirdparty/squid.git/commit - src/Transients.cc
Fixed StoreMap.cc "anchorAt(anchorId).reading()" assertions (#1117)
authorAlex Rousskov <rousskov@measurement-factory.com>
Sun, 21 Aug 2022 05:30:25 +0000 (05:30 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Mon, 22 Aug 2022 18:46:25 +0000 (18:46 +0000)
commit24c937808c28bc755cdce682d624bc7c4225a227
treec59926727755ef062c31f4fd484c197ebee7c112
parentc81de627914ae4a943fbedc10e880555b508a059
Fixed StoreMap.cc "anchorAt(anchorId).reading()" assertions (#1117)

Squid asserted because the code could switch a transient entry from
writing to reading while the corresponding store entry was still being
(or could still be) written to the shared memory cache. For example:

1. We start as a collapsed writer.
2. We get a response and start writing to disk and shared memory caches.
3. Disk swapout fails (for any reason, including out-of-slots errors).
4. storeSwapOutFileClosed() calls transientsCompleteWriting().
5. transientsCompleteWriting() switches the entry into reading mode
   ... but we are still writing to the memory cache!

There was a somewhat related XXX in transientsCompleteWriting(), but
what that comment did not say is that if we are writing to two stores in
parallel, then the first transientsCompleteWriting() call (made when one
of the two swapouts ends) makes us a reader prematurely.

An incorrect reader status allows a Controller::syncCollapsed()
notification (to, say, a shared memory cache writer) slip past the
transients->isWriter() guard, eventually reaching the reading()
assertion.

Properly detecting the end of all store writing is difficult because the
two mostly independent writing "threads" may start/finish at seemingly
random times, in many code places, and even in different workers. To
simplify this task, Squid now limits cache writing to the worker
transaction that made the StoreEntry object public. That transaction
creates a writing Transients entry. Other transactions start as
Transients readers. The writer switches to reader when neither memory
nor disk caching can start or continue.

Also simplified Transients entry state. Squid relayed swapout errors via
the Transient entries themselves, but that is not necessary (and it
prevented the cache entry from recovering by writing to another store).
Each store entry can take care of its own swapout status/results. The
readers just need to know whether somebody may start (or is still)
writing, and we relay that info by keeping the Transients entry locked
for writing (appending, to be precise) while that condition is true.

Also fixed shared caches to recognize that no more data will be coming
in because the remote entry writer is gone. Readers still try to deliver
what they have, even if they know that the response will be truncated.

Also tried to follow the "broadcast after change, in the same context as
the change" principle in the modified code instead of relying on the
caller to broadcast after all changes. This approach may increase the
number of broadcasts, but it reduces the probability that we will miss
an important Broadcast() call. We can (and should) optimize repeated,
useless broadcasts, but that work is outside this project scope.

Also improved StoreEntry::swapOut() shutdown safety and
mayStartSwapOut() checks descriptions/order.

Also added an out-of-scope XXX.
src/MemStore.cc
src/Store.h
src/Transients.cc
src/Transients.h
src/fs/rock/RockIoState.cc
src/fs/rock/RockSwapDir.cc
src/store.cc
src/store/Controller.cc
src/store/Controller.h
src/store_swapout.cc
src/tests/stub_libstore.cc