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

Reply via email to