Re: [PATCH] Fixes to get more collapsed forwarding hits

On 24/06/2014 4:07 p.m., Alex Rousskov wrote:
> Hello,
>
> The attached patch contains several changes to improve the
> probability of getting a collapsed forwarding hit:
>
> * Allow STORE_MEMORY_CLIENTs to open disk files if needed and possible.
> STORE_*_CLIENT designation is rather buggy (several known XXXs). Some
> collapsed clients are marked as STORE_MEMORY_CLIENTs (for the lack of
> info at determination time) but their hit content may actually come from
> a disk cache.
>
> * Do not abandon writing a collapsed cache entry when we cannot cache
> the entry in RAM if the entry can be cached on disk instead. Both shared
> memory cache and the disk cache have to refuse to cache the entry for it
> to become non-collapsible. This dual refusal is difficult to detect
> because each cache may make the caching decision at different times.
> Added StoreEntry methods to track those decisions and react to them.

Hmm. FTR: IMHO the only reasons a response should be non-collapsible is
if it is a) private, b) authenticated to another user, c) explicitly
known non-cacheable, d) a non-matching variant of the URL, or e) we
already discarded some body bytes.

The factor of memory/disk caches not having provided cacheable size
allow/reject result yet is no reason to be non-collapsible. It is a
reason to wait on that result to determine the cacheability condition (c).

>
> * Recognize disk cache as a potential source of the collapsed entry when
> the memory cache is configured. While collapsed entries would normally
> be found in the shared memory cache, caching policies and other factors
> may prohibit memory caching but still allow disk caching. Memory cache
> is still preferred.
>
> * Do not use unknown entry size in StoreEntry::checkTooSmall()
> determination. The size of collapsed entries is often unknown, even when
> they are STORE_OK (because swap_hdr_sz is unknown when the other worker
> has created the cache entry). The same code has been using this
> ignore-unknowns logic for the Content-Length header value, so the
> rejection of unknown entry size (added as a part of C++ conversion
> without a dedicated message in r5766) could have been a typo.
>
>
> If approved, I will commit the change for the last bullet separately
> because it is easier to isolate and is less specific to collapsed
> forwarding.
>
>
> The patch does not attempt to cover all possible collapsing cases that
> Squid currently misses, of course. More work remains in this area
> (including a serious STORE_*_CLIENT redesign), but the proposed fixes
> should make a dent.
>
> These changes were developed for an earlier trunk version but the
> trunk-ported patch posted here also passed simple lab tests.
>
>
> Thank you,
>
> Alex.
>

+1. Although I do not know enough about store yet to do much in-depth
audit of the logic changes.

Amos
Received on Tue Jun 24 2014 - 05:58:42 MDT