Re: [PATCH] Large Rock and Collapsed Forwarding

On 1/01/2014 8:40 a.m., Alex Rousskov wrote:
> Hello,
>
> The attached patch contains initial Large Rock and Collapsed
> Forwarding support, also available as a Launchpad branch:
> lp:~measurement-factory/squid/collapsed-fwd/
>
> Large Rock: Support disk (and shared memory) caching of responses
> exceeding one db slot (or one shared memory page) in size. A single db
> slot/page size is still limited to 32KB (smaller values can be
> configured for disk caches using the newly added cache_dir slot-size
> option). Removal of old rock cache dir (followed by squid-z) is required
> -- the on-disk db structure has changed.
>
> Collapsed Forwarding: Optionally merge concurrent cachable requests for
> the same URI earlier: After the request headers have been parsed (as
> before), but now _before_ the response headers have been received.
> Merging of requests received by different SMP workers is supported.
> Controlled by the new collapsed_forwarding directive in squid.conf.
> Disabled by default because all but one of the merged requests have to
> be delayed (until the response headers are received) for the merging to
> work, which may be worse than forwarding all concurrent requests
> immediately. The overall feature idea and request eligibility conditions
> are based on Collapsed Forwarding in Squid2.
>
> Here is a summary of other important changes (the branch log contains
> the details and I propose that the branch is merged to preserve them):
>
> * Tightened StoreEntry locking. Split StoreEntry::lock() into "just
> lock" and "update entry reference time" interfaces, addressing an old
> XXX. Improved entry lock/unlock debugging. Needs more work.
>
> * Adjusted StoreIOState::write() API to allow callers detect write errors.
>
> * Simplified MemObject::write() API to remove an essentially unused
> callback.
>
> * Mark client streams that sent everything as STREAM_COMPLETE. The old
> code used STREAM_UNPLANNED_COMPLETE if the completed stream was
> associated with a non-persistent connection, which did not make sense to
> me and, IIRC, led to store entry aborts even though the entries were not
> damaged in any way.
>
> * mem_hdr::hasContigousContentRange() now returns true for empty ranges.
>
> * Support "appending" ReadWriteLock state that can be shared by readers
> and the writer. The writer promises not to update key metadata (except
> growing object size and next pointers) and readers promise to be careful
> when reading growing slices.
>
> * Fixed StoreEntry::mayStartSwapOut() logic to handle terminated swapouts.
>
> * Improved STORE_MEM_CLIENT detection and documented known (and mostly
> old) StoreEntry::storeClientType() problems.
>
> * Removed StoreEntry::hidden_mem_obj hack.
>
> * Polished StoreEntry debugging to report more info, less noise. Use e:
> prefix.
>
> * Added a script to extract store entry(ies) debugging from cache.log.
> Needs more work.
>
>
> The latest code passes built-in Squid test cases, some of its revisions
> have been independently tested in at least two labs, and its earlier
> incarnations have been successfully deployed in at least one production
> environment. However, given their size and complexity, the proposed
> changes are likely to introduce at least some instability and
> regressions if accepted to trunk. We will need to monitor and address
> those problems as needed.
>
>
> This patch does not contain several large/important Store changes that
> we still should make [soon], including:
>
> * Optimize rock db loading when Squid starts.
>
> * Support Vary caching in shared memory.
>
> * Fully support cached HTTP response header updates.
>
> * Stop using the Store class as a kitchen sink for Store-related APIs.
>
> * Better names and clear role separation for classes responsible for
> memory and disk caches (where "disk cache" is "all cache_dirs taken
> together and treated as a single cache").
>
> * Migration of store_table global into something that better integrates
> with SMP needs (probably similar to the proposed Transients but without
> SMP overheads).
>
> * Refcounting of StoreEntries.
>
>
> Given the current demand for Large Rock and Collapsed Forwarding
> changes, combined with the amount of time it will take to implement the
> missing changes listed above, and the size/complexity of the already
> implemented changes, I think it is better to merge the implemented
> changes now than wait for more changes to come.
>
> An earlier version of this code was posted to squid-dev as a PREVIEW a
> few months ago. Amos kindly reviewed that posting. My response to his
> review is below.
>

Thank you for addressing those earlier comments.

I've taken a stab at reading through this massive patch again. But too
much of it is in areas of code I dont already know well enough to
identify bugs, and as usual you dont leave any simple issues to point at :-)

I did spot:

in src/tests/stub_CollapsedForwarding.cc:
 * please remove the copyright blurb. Did Robert really design that stub
for you?

+1. IMHO, if nobody else wants to take a stab at a full audit I think
you should just merge this and we fix the bugs as found.

Amos
Received on Wed Jan 01 2014 - 14:35:18 MST