Re: [PATCH] Re: r13295: cache_mem + rock store = "memCopy: could not find start of [x,y) in memory"

On 2014-03-20 03:27, Alex Rousskov wrote:
> Hello,
>
> The attached patch avoids "FATAL: Squid has attempted to read data
> from memory that is not present" crashes when shared memory caching is
> enabled in trunk. It also improves related code.
>
> Shared memory caching code was not checking whether the response is
> generally cachable and, hence, tried to store responses that Squid
> already refused to cache (e.g., release-requested entries). The shared
> memory cache should also check that the response is contiguous because
> the disk store may not check that (e.g., when disk caching is
> disabled).
>
> The problem was exacerbated by the broken entry dump code accompanying
> the FATAL message. The Splay tree iterator is evidently not iterating a
> portion of the tree. I added a visitor pattern code to work around
> that,
> but did not try to fix the Splay iterator itself because, in part, the
> whole iterator design felt inappropriate (storing and flattening the
> tree before iterating it!?) for its intended purpose. I could not check
> quickly enough where the broken iterator is used besides
> mem_hdr::debugDump() so more bugs like this are possible. Checking and
> fixing this would be a nice compact project for a volunteer looking for
> such projects.

SplayNode already has a C-ish walk(SPLAYWALKEE*) visitor pattern. Would
you be open to merging the two and committing the splay parts
separately?

If not (or not yet) please add a XXX note and/or an entry to the wiki
RoadMap/Tasks list about it.

>
> Optimized StoreEntry::checkCachable() a little and removed wrong TODO
> comment: Disk-only mayStartSwapOut() should not accumulate "general"
> cachability checks which are used by RAM caches as well.
>
> Added more mem_hdr debugging and polished method descriptions.
>
> Fixed NullStoreEntry::mayStartSwapout() spelling/case. It should
> override StoreEntry::mayStartSwapOut().

Look simple enough to +1.

>
>
> The patch is against trunk with Vector migration removed (to avoid
> random crashes) and fatal() replaced with fatal_dump() as discussed on
> this thread. I will commit the fatal_dump() change separately if nobody
> beats me to it.
>

Amos
Received on Wed Mar 19 2014 - 21:44:42 MDT