From 97754f5abcae0b186d4a9f92bb7fd388e76ef8a5 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 18 Mar 2014 22:04:52 -0600 Subject: [PATCH] Avoid "FATAL: Squid has attempted to read data from memory that is not present" crashes. Improve 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 id 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 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. 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(). --- include/splay.h | 26 ++++++++++++++++++++++++++ src/MemStore.cc | 7 ++++++- src/MemStore.h | 2 +- src/Store.h | 7 +++++-- src/SwapDir.h | 2 +- src/stmem.cc | 4 +++- src/store.cc | 22 +++++++++++++++++----- src/store_dir.cc | 2 +- src/tests/stub_store.cc | 2 +- 9 files changed, 61 insertions(+), 13 deletions(-) diff --git a/include/splay.h b/include/splay.h index ea4f2c3e03..d4e4202fe8 100644 --- a/include/splay.h +++ b/include/splay.h @@ -31,6 +31,9 @@ public: SplayNode * insert(Value data, SPLAYCMP * compare); template SplayNode * splay(const FindValue &data, int( * compare)(FindValue const &a, Value const &b)) const; + + /// recursively visit left nodes, this node, and then right nodes + template void visit(Visitor &v) const; }; typedef SplayNode splayNode; @@ -71,6 +74,9 @@ public: const_iterator end() const; + /// recursively visit all nodes, in left-to-right order + template void visit(Visitor &v) const; + size_t elements; }; @@ -274,6 +280,25 @@ SplayNode::splay(FindValue const &dataToFind, int( * compare)(FindValue const return top; } +template +template +void +SplayNode::visit(Visitor &visitor) const { + if (left) + left->visit(visitor); + visitor(data); + if (right) + right->visit(visitor); +} + +template +template +void +Splay::visit(Visitor &visitor) const { + if (head) + head->visit(visitor); +} + template template typename Splay::Value const * @@ -360,6 +385,7 @@ Splay::end() const return const_iterator(NULL); } +// XXX: This does not seem to iterate the whole thing in some cases. template class SplayConstIterator { diff --git a/src/MemStore.cc b/src/MemStore.cc index 29c1917418..f327bf1e50 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -411,7 +411,7 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof) /// whether we should cache the entry bool -MemStore::shouldCache(const StoreEntry &e) const +MemStore::shouldCache(StoreEntry &e) const { if (e.mem_status == IN_MEMORY) { debugs(20, 5, "already loaded from mem-cache: " << e); @@ -453,6 +453,11 @@ MemStore::shouldCache(const StoreEntry &e) const return false; // will not cache due to cachable entry size limits } + if (!e.mem_obj->isContiguous()) { + debugs(20, 5, "not contiguous"); + return false; + } + if (!map) { debugs(20, 5, HERE << "No map to mem-cache " << e); return false; diff --git a/src/MemStore.h b/src/MemStore.h index c536620863..e67cc4a506 100644 --- a/src/MemStore.h +++ b/src/MemStore.h @@ -58,7 +58,7 @@ public: static int64_t EntryLimit(); protected: - bool shouldCache(const StoreEntry &e) const; + bool shouldCache(StoreEntry &e) const; bool startCaching(StoreEntry &e); void copyToShm(StoreEntry &e); diff --git a/src/Store.h b/src/Store.h index 241667358b..6a4c191c2b 100644 --- a/src/Store.h +++ b/src/Store.h @@ -120,11 +120,14 @@ public: bool swappingOut() const { return swap_status == SWAPOUT_WRITING; } void swapOutFileClose(int how); const char *url() const; + /// generally cachable (checks agnostic to disk/RAM-specific requirements) + /// common part of mayStartSwapOut() and memoryCachable() + /// TODO: Make private so only those two methods can call this. int checkCachable(); int checkNegativeHit() const; int locked() const; int validToSend() const; - bool memoryCachable() const; ///< may be cached in memory + bool memoryCachable(); ///< checkCachable() and can be cached in memory /// if needed, initialize mem_obj member w/o URI-related information MemObject *makeMemObject(); @@ -272,7 +275,7 @@ private: store_client_t storeClientType() const {return STORE_MEM_CLIENT;} char const *getSerialisedMetaData(); - bool mayStartSwapout() {return false;} + virtual bool mayStartSwapOut() { return false; } void trimMemory(const bool preserveSwappable) {} diff --git a/src/SwapDir.h b/src/SwapDir.h index bd1e542c05..1dd99812e7 100644 --- a/src/SwapDir.h +++ b/src/SwapDir.h @@ -104,7 +104,7 @@ public: private: void createOneStore(Store &aStore); StoreEntry *find(const cache_key *key); - bool keepForLocalMemoryCache(const StoreEntry &e) const; + bool keepForLocalMemoryCache(StoreEntry &e) const; bool anchorCollapsed(StoreEntry &collapsed, bool &inSync); bool anchorCollapsedOnDisk(StoreEntry &collapsed, bool &inSync); diff --git a/src/stmem.cc b/src/stmem.cc index 011dca0463..755a363420 100644 --- a/src/stmem.cc +++ b/src/stmem.cc @@ -95,6 +95,7 @@ mem_hdr::unlink(mem_node *aNode) return false; } + debugs(19, 8, this << " removing " << aNode); nodes.remove (aNode, NodeCompare); delete aNode; return true; @@ -103,6 +104,7 @@ mem_hdr::unlink(mem_node *aNode) int64_t mem_hdr::freeDataUpto(int64_t target_offset) { + debugs(19, 8, this << " up to " << target_offset); /* keep the last one to avoid change to other part of code */ SplayNode const * theStart; @@ -232,7 +234,7 @@ mem_hdr::debugDump() const debugs (19, 0, "mem_hdr::debugDump: lowest offset: " << lowestOffset() << " highest offset + 1: " << endOffset() << "."); std::ostringstream result; PointerPrinter foo(result, " - "); - for_each (getNodes().begin(), getNodes().end(), foo); + getNodes().visit(foo); debugs (19, 0, "mem_hdr::debugDump: Current available data is: " << result.str() << "."); } diff --git a/src/store.cc b/src/store.cc index 066cb456a2..916aad2b90 100644 --- a/src/store.cc +++ b/src/store.cc @@ -949,11 +949,23 @@ StoreEntry::checkTooSmall() return 0; } -// TODO: remove checks already performed by swapoutPossible() // TODO: move "too many open..." checks outside -- we are called too early/late int StoreEntry::checkCachable() { + // XXX: This method is used for both memory and disk caches, but some + // checks are specific to disk caches. Move them to mayStartSwapOut(). + + // XXX: This method may be called several times, sometimes with different + // outcomes, making store_check_cachable_hist counters misleading. + + // check this first to optimize handling of repeated calls for uncachables + if (EBIT_TEST(flags, RELEASE_REQUEST)) { + debugs(20, 2, "StoreEntry::checkCachable: NO: not cachable"); + ++store_check_cachable_hist.no.not_entry_cachable; // TODO: rename? + return 0; // avoid rerequesting release below + } + #if CACHE_ALL_METHODS if (mem_obj->method != Http::METHOD_GET) { @@ -964,9 +976,6 @@ StoreEntry::checkCachable() if (store_status == STORE_OK && EBIT_TEST(flags, ENTRY_BAD_LENGTH)) { debugs(20, 2, "StoreEntry::checkCachable: NO: wrong content-length"); ++store_check_cachable_hist.no.wrong_content_length; - } else if (EBIT_TEST(flags, RELEASE_REQUEST)) { - debugs(20, 2, "StoreEntry::checkCachable: NO: not cachable"); - ++store_check_cachable_hist.no.not_entry_cachable; // TODO: rename? } else if (EBIT_TEST(flags, ENTRY_NEGCACHED)) { debugs(20, 3, "StoreEntry::checkCachable: NO: negative cached"); ++store_check_cachable_hist.no.negative_cached; @@ -1400,8 +1409,11 @@ storeConfigure(void) } bool -StoreEntry::memoryCachable() const +StoreEntry::memoryCachable() { + if (!checkCachable()) + return 0; + if (mem_obj == NULL) return 0; diff --git a/src/store_dir.cc b/src/store_dir.cc index 82d6d8324c..8d124736b8 100644 --- a/src/store_dir.cc +++ b/src/store_dir.cc @@ -865,7 +865,7 @@ void StoreController::markForUnlink(StoreEntry &e) // move this into [non-shared] memory cache class when we have one /// whether e should be kept in local RAM for possible future caching bool -StoreController::keepForLocalMemoryCache(const StoreEntry &e) const +StoreController::keepForLocalMemoryCache(StoreEntry &e) const { if (!e.memoryCachable()) return false; diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc index 72b94c94af..fd2bb4819a 100644 --- a/src/tests/stub_store.cc +++ b/src/tests/stub_store.cc @@ -50,7 +50,7 @@ int StoreEntry::checkCachable() STUB_RETVAL(0) int StoreEntry::checkNegativeHit() const STUB_RETVAL(0) int StoreEntry::locked() const STUB_RETVAL(0) int StoreEntry::validToSend() const STUB_RETVAL(0) -bool StoreEntry::memoryCachable() const STUB_RETVAL(false) +bool StoreEntry::memoryCachable() STUB_RETVAL(false) MemObject *StoreEntry::makeMemObject() STUB_RETVAL(NULL) void StoreEntry::createMemObject(const char *, const char *, const HttpRequestMethod &aMethod) STUB void StoreEntry::dump(int debug_lvl) const STUB -- 2.39.5