[squid-dev] [PATCH] Bug 7: Headers are not updated on disk after 304s

Sat Mar 12 17:39:21 UTC 2016

I will try to follow up at:
http://bugs.squid-cache.org/show_bug.cgi?id=7

Eliezer

On 11/03/2016 20:16, Alex Rousskov wrote:
> On 03/11/2016 02:17 AM, Amos Jeffries wrote:
>> On 11/03/2016 2:59 p.m., Alex Rousskov wrote:
>>>      The attached compressed patch fixes a 15+ years old Bug #7 [1] for
>>> the shared memory cache and rock cache_dirs. I am not aware of anybody
>>> working on ufs-based cache_dirs, but this patch provides a Store API and
>>> a cache_dir example on how to fix those as well.
>>>
>>>    [1] http://bugs.squid-cache.org/show_bug.cgi?id=7
>
>
>> Ah. I'm getting deja-vu on this. Thought those two cache types were
>> fixed long ago and recent talk was you were working on the UFS side of it.
>
> There was some noise about this bug and related issues some months ago.
> It was easy to get confused by all the mis[leading]information being
> posted on bugzilla, including reports that "the bug is fixed" for some
> ufs-based cache_dirs. I tried to correct those reports but failed to
> convince people that they do not see what they think they see.
>
> After this patch, the following cache stores (and only them) should
> support header updates:
>
>    * non-shared memory cache (in non-SMP Squids only)
>    * shared memory cache
>    * rock cache_dir
>
> Needless to say, the posted patch does not fix all the problems with
> header updates, even for the above stores. For example, the code that
> decides which headers to update may still violate HTTP in some ways (I
> have not checked). The patch "just" writes the headers computed by Squid
> to shared memory cache and to rock cache_dirs.
>
> Moreover, given the [necessary] complexity of the efficient update code
> combined with the [unnecessary] complexity of some old Store APIs, I
> would be surprised if there are no new bugs or problems introduced by
> our changes. I am not aware of any, but we continue to test and plan to
> fix the ones we find.
>
>
>>> Besides unavoidable increase in rock-based caching code complexity, the
>>> [known] costs of this fix are:
>>>
>>> 1. 8 additional bytes per cache entry for shared memory cache and rock
>>> cache_dirs. Much bigger but short-lived RAM _savings_ for rock
>>> cache_dirs (due to less RAM-hungry index rebuild code) somewhat mitigate
>>> this RAM usage increase.
>>>
>>> 2. Increased slot fragmentation when updated headers are slightly larger
>>> than old ones. This can probably be optimized away later if needed by
>>> padding HTTP headers or StoreEntry metadata.
>>>
>>> 3. Somewhat slower rock cache_dir index rebuild time. IMO, this should
>>> eventually be dealt with by not rebuilding the index on most startups at
>>> all (rather than focusing on the index rebuild optimization).
>>
>> Hmm. Nod, agreed on the long-term approach.
>>
>>>
>>> The patch preamble (also quoted below) contains more technical details,
>>> including a list of side changes that, ideally, should go in as separate
>>> commits. The posted patch is based on our bug7 branch on lp[2] which has
>>> many intermediate commits. I am not yet sure whether it makes sense to
>>> _merge_ that branch into trunk or simply commit it as a single/atomic
>>> change (except for those side changes). Opinions welcomed.
>
>
>> Do you know how to do a merge like that with bzr properly?
>>   My experience has been that it only likes atomic-like merges.
>
> I sense a terminology conflict. By "merge", I meant "bzr merge". Trunk
> already has many merged branches, of course:
>
>    revno: 14574 [merge]
>    revno: 14573 [merge]
>    revno: 14564 [merge]
>    ...
>
> By single/atomic change, I meant "patch < bug7.patch". Merges preserve
> individual branch commits which is good when those commits are valuable
> and bad when those commits are noise. In case of our bug7 branch, it is
> a mixture of valuable stuff and noise. I decided to do a single/atomic
> change to avoid increasing the noise level.
>
>
>> in src/StoreIOState.h:
>>
>> * if the XXX about file_callback can the removal TODO be enacted ?
>>   - at least as one of the side-change patches
>
> Yes, of course, but out of this project scope. We already did the
> difficult part -- detected and verified that the API is unused.
> Hopefully, somebody will volunteer to do the rest (and to take the
> responsibility for it).
>
>
>> * the docs on touchingStoreEntry() seem to contradict your description
>> of how the entry chains work. Now. You said readers could read whatever
>> chain they were attached to after the update switch. The doc says they
>> only ever read the primary.
>
> Done: Clarified that the primary chain (which the readers always start
> with) may become secondary later:
>
>>      // Tests whether we are working with the primary/public StoreEntry chain.
>>      // Reads start reading the primary chain, but it may become secondary.
>>      // There are two store write kinds:
>>      // * regular writes that change (usually append) the entry visible to all and
>>      // * header updates that create a fresh chain (while keeping the stale one usable).
>>      bool touchingStoreEntry() const;
>
> The readers do not matter in the current code because reading code does
> not use this method, but that may change in the future, of course.
>
>
>> in src/fs/rock/RockHeaderUpdater.cc:
>>
>> * please remove the dead include for cache_cf.h
>
> Done.
>
>
>> * missing copyright blurb
>
> Done.
>
>
>> * since the dtor is just {} can it go in the header ?
>>   - possibly using "= default"
>
> Done.
>
>
>> in src/fs/rock/RockHeaderUpdater.h:
>>
>> * '{' on the line after 'class' please.
>
> Done. I am surprised this is not handled automatically by astyle!
>
>
>>   - there might be others.
>
> Done? I found two more.
>
>
>> * CBDATA_CLASS at the top of the class please.
>
> Done.
>
>
>> in src/fs/rock/RockSwapDir.cc:
>>
>> * 2 new HERE being added in Rock::SwapDir::createUpdateIO()
>
> Done.
>
>
>> in src/ipc/ReadWriteLock.cc:
>>
>> * the new Ipc::AssertFlagIsSet() would be dangerous for a reader to use
>> on a read flag.
>
> Yes, as dangerous as any assert().
>
>
>>   - I think the API docs need an extra warning that it sets the flag,
>> which may or may not need un-setting later.
>
> There will be no "later": If the flag was not set, Squid asserts (and
> sets the flag as a side effect). This is a "flag is set" assertion, not
> a function that sets the flag or retrieves the current flag value (the
> latter cannot be done without side effects AFAICT).
>
> To partially address your concern, I changed this function to return
> nothing and detailed the function description:
>
>> /// Same as assert(flag is set): The process assert()s if flag is not set.
>> /// Side effect: The unset flag becomes set just before we assert().
>> /// Needed because atomic_flag cannot be compared with a boolean.
>> void AssertFlagIsSet(std::atomic_flag &flag);
>
>
> I do not like this function at all, but AFAICT, this is the price to pay
> for using std::atomic_flag optimization. If you prefer, we can delete
> that C++11 optimization and use the good old atomic<bool> instead.
>
>
>> in src/MemStore.cc:
>>
>> * the XXX about Packable API is misplaced. If you want to add, it should
>> be in Packable.h
>
> Done, with one more XXX added to Packable.h since we are changing it
> anyway. I committed this Packable.h change separately.
>
>
>> * docs on MemStore::pageForSlice() should use doxygen
>
> Done.
>
>
>> in src/ipc/StoreMap.cc:
>>
>> * is "cannot open freshless entry" a typo of "keyless" ?
>
> Changed to "freshchainless" to clarify. Better adjectives welcomed.
>
> The pattern of all these failure debugging lines in openFoo() methods is
> (or should be) "cannot open ADJECTIVE entry" where the adjective
> describes the characteristic of the entry that prevents us from opening
> it: "busy", "empty", "marked[ for deletion]", etc.
>
> In this specific case, the entry cannot be opened for updates because
> Squid cannot allocate a fresh chain for it. This is why I invented the
> "freshless" adjective to mean "without a fresh chain". Note that the
> "bad" entry in question is the old stale entry, not the fresh [keyless]
> chain which we failed to create.
>
>
>> That all seems relatively small, so +1.
>
> Committed as trunk revisions 14580-14584. I did use "--fixes squid:7"
> with "bzr commit" even though this is a partial fix. Sorry of that was
> wrong.
>
>
> Thank you,
>
> Alex.
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>