]> git.ipfire.org Git - thirdparty/squid.git/commit - src/store/Disks.h
Bug 4505: SMP caches sometimes do not purge entries (#46)
authorEduard Bagdasaryan <dictiano@gmail.com>
Fri, 2 Feb 2018 20:41:40 +0000 (23:41 +0300)
committerAlex Rousskov <rousskov@measurement-factory.com>
Fri, 2 Feb 2018 20:41:40 +0000 (13:41 -0700)
commit4310f8b00dd574542dcec4208112bb89ef403528
tree6af0e519787bf5ce1f7c54f09179ba581d81b703
parent613a4324127964daec3d24fbea4fc2826dae84f4
Bug 4505: SMP caches sometimes do not purge entries (#46)

When Squid finds a requested entry in the memory cache, it does not
check whether the same entry is also stored in a cache_dir. The
StoreEntry object may become associated with its store entry in the
memory cache but not with its store entry on disk. This inconsistency
causes two known problems:

1. Squid may needlessly swap out the memory hit to disk, either
   overwriting an existing (and identical) disk entry or, worse,
   creating a duplicate entry on another disk. In the second case, the
   two disk entries are not synchronized and may eventually start to
   differ if one of them is removed or updated.

2. Squid may not delete a stale disk entry when needed, violating
   various HTTP MUSTs, and eventually serving stale [disk] cache entries
   to clients.

Another purging problem is not caused by the above inconsistency:

3. A DELETE request or equivalent may come for the entry which is still
   locked for writing. Squid fails to get a lock for such an entry (in
   order to purge it) and the entry remains in disk and/or memory cache.

To solve the first two problems:

* StoreEntry::mayStartSwapout() now avoids needless swapouts by checking
  whether StoreEntry was fully loaded, is being loaded, or could have
  been loaded from disk. To be able to reject swapouts in the last case,
  we now require that the newer (disk) entries explicitly delete their
  older variants instead of relying on the Store to overwrite the older
  (unlocked) variant. That explicit delete should already be happening
  in higher-level code (that knows which entry is newer and must mark
  any stale entries for deletion anyway).

To fix problem #3:

* A new Store::Controller::evictIfFound(key) method purges (or marks for
  deletion if purging is impossible) all the matching store entries,
  without loading the StoreEntry information from stores. Avoiding
  StoreEntry creation reduces waste of resources (the StoreEntry object
  would have to be deleted anyway) _and_ allows us to mark being-created
  entries (that are locked for writing and, hence, cannot be loaded into
  a StoreEntry object).

XXX: SMP cache purges may continue to malfunction when the Transients
table is missing. Currently, Transients are created only when the
collapsed_forwarding is on. After Squid bug 4579 is fixed, every public
StoreEntry will have the corresponding Transients entry and vice versa,
extending these fixes to all SMP environments.

Note that even if Squid properly avoids storing duplicate disk entries,
some cache_dir manipulations by humans and Squid crashes may lead to
such duplicates being present.  This patch leaves dealing with potential
duplicates out of scope except it guarantees that if an entry is
deleted, then all [possible] duplicates are deleted as well.

Fixing the above problems required (and/or benefited from) many related
improvements, including some Store API changes. It is impractical to
detail each change here, but several are highlighted below.

To propagate DELETEs across workers, every public StoreEntry now has a
Transients entry.

Prevented concurrent cache readers from aborting when their entry is
release()d. Unlike abort, release should not affect current readers.

Fixed store.log code to avoid "Bug: Missing MemObject::storeId value".

Removed Transients extras used to initialize MemObject StoreID/method in
StoreEntry objects created by Transients::get() for collapsed requests.
Controlled::get() and related Controller APIs do not _require_ setting
those MemObject details: get() methods for all cache stores return
StoreEntry objects without them (because entry basics lack Store ID and
request method). The caller is responsible for cache key collision
detection. Controlled::get() parameters could include Store ID and
request method for early cache key collision detection, but adding a
StoreQuery class and improving collision detection code is outside this
project scope (and requires many changes).

Found more cases where release() should not prevent sharing.
Remaining cases need further analysis as discussed in master 39fe14b2.

Greatly simplified UFS store rebuilding, possibly fixing subtle bug(s).

Clarified RELEASE_REQUEST flag meaning, becoming 'a private StoreEntry
which can't become public anymore'. Refactored the related code,
combining two related notions: 'a private entry' and 'an entry marked
for removal'.

Do not abort collapsed StoreEntries during syncing just because the
corresponding being stored shared entry was marked for deletion. Abort
them if the shared entry has been also aborted.

Added StoreEntry helper methods to prevent direct manipulation of
individual disk-related data members (swap_dirn, swap_filen, and
swap_status). These methods help keep these related data members in a
coherent state and minimize code duplication.
50 files changed:
src/CollapsedForwarding.cc
src/CollapsedForwarding.h
src/MemObject.cc
src/MemObject.h
src/MemStore.cc
src/MemStore.h
src/Store.h
src/Transients.cc
src/Transients.h
src/client_side_reply.cc
src/clients/FtpGateway.cc
src/enums.h
src/fs/rock/RockSwapDir.cc
src/fs/rock/RockSwapDir.h
src/fs/ufs/RebuildState.cc
src/fs/ufs/RebuildState.h
src/fs/ufs/UFSSwapDir.cc
src/fs/ufs/UFSSwapDir.h
src/gopher.cc
src/http.cc
src/ipc/StoreMap.cc
src/ipc/StoreMap.h
src/mime.cc
src/neighbors.cc
src/peer_digest.cc
src/store.cc
src/store/Controlled.h
src/store/Controller.cc
src/store/Controller.h
src/store/Disk.cc
src/store/Disk.h
src/store/Disks.cc
src/store/Disks.h
src/store/Storage.h
src/store/forward.h
src/store_client.cc
src/store_log.cc
src/store_rebuild.cc
src/store_rebuild.h
src/store_swapin.cc
src/store_swapout.cc
src/tests/TestSwapDir.h
src/tests/stub_CollapsedForwarding.cc
src/tests/stub_MemStore.cc
src/tests/stub_store.cc
src/tests/stub_store_rebuild.cc
src/tests/testRock.cc
src/tests/testStoreController.cc
src/tests/testStoreHashIndex.cc
src/whois.cc