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

[Alex Rousskov]

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.