From: Julien Pinon Date: Sat, 21 Jul 2012 04:05:25 +0000 (-0600) Subject: Bug 3462: Delay Pools and ICAP X-Git-Tag: SQUID_3_1_21~15 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8e21037d844e9ec5402eb515d5548de78c13b5aa;p=thirdparty%2Fsquid.git Bug 3462: Delay Pools and ICAP Allow StoreEntry::bytesWanted() API to ignore delay pools. Use that feature when shoveling adapted response body from ICAP/eCAP BodyPipe into Store. If we do not ignore delay pools in ServerStateData::handleMoreAdaptedBodyAvailable() context, and our pool quota is exhausted, we will stop reading from the adaptation BodyPipe, but there is no code/API to notify a BodyPipe reader when the quota becomes available again. With no reads from the pipe, the ICAP service stops sending more adapted body data and possibly stops reading the virgin body data from Squid, resulting in a stuck ICAP RESPMOD and the master HTTP transaction. We do need to call StoreEntry::bytesWanted() in this context because otherwise Squid may run out of RAM (Squid bug #2619). The fix for that problem inadvertently created this bug when delay pools were enabled. Long-term, a "kick BodyPipe consumer when there is quota" code should be added, and delay pools should be enabled for ICAP responses, but with enough knobs for admins to disable ICAP pooling where needed. Most ICAP environments will probably want to disable bandwidth controls because ICAP service traffic is usually "local". Removed StoreEntry::bytesWanted() TRY_FWD_HDR_WAIT guard that disabled delay pool application until the final HTTP response headers are received (HttpStateData::haveParsedReplyHeaders() is called). Nobody could fully explain why that condition was really needed (and we obviously want to limit bandwidth consumption at all response processing stages, in general). The now-removed guard was bypassing delay pool checks for virgin HTTP response (until the ICAP server responded with adapted headers), causing bandwidth overuse. Possibly fixes or helps with Squid bug #2606 as well. --- diff --git a/src/MemObject.cc b/src/MemObject.cc index e497edce92..4684332334 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -376,16 +376,17 @@ MemObject::isContiguous() const } int -MemObject::mostBytesWanted(int max) const +MemObject::mostBytesWanted(int max, bool ignoreDelayPools) const { #if DELAY_POOLS - /* identify delay id with largest allowance */ - DelayId largestAllowance = mostBytesAllowed (); - return largestAllowance.bytesWanted(0, max); -#else + if (!ignoreDelayPools) { + /* identify delay id with largest allowance */ + DelayId largestAllowance = mostBytesAllowed (); + return largestAllowance.bytesWanted(0, max); + } +#endif return max; -#endif } void diff --git a/src/MemObject.h b/src/MemObject.h index 5464ccb197..dedf8f50d3 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -77,7 +77,7 @@ public: void trimSwappable(); void trimUnSwappable(); bool isContiguous() const; - int mostBytesWanted(int max) const; + int mostBytesWanted(int max, bool ignoreDelayPools) const; void setNoDelay(bool const newValue); #if DELAY_POOLS diff --git a/src/Server.cc b/src/Server.cc index 9c597a9abb..1c28d885b3 100644 --- a/src/Server.cc +++ b/src/Server.cc @@ -721,7 +721,7 @@ ServerStateData::handleMoreAdaptedBodyAvailable() if (!contentSize) return; // XXX: bytesWanted asserts on zero-size ranges - const size_t spaceAvailable = entry->bytesWanted(Range(0, contentSize)); + const size_t spaceAvailable = entry->bytesWanted(Range(0, contentSize), true); if (spaceAvailable < contentSize ) { // No or partial body data consuming diff --git a/src/Store.h b/src/Store.h index 0b7ea9d0ea..cd3d06601e 100644 --- a/src/Store.h +++ b/src/Store.h @@ -79,7 +79,7 @@ public: virtual void write (StoreIOBuffer); virtual _SQUID_INLINE_ bool isEmpty() const; virtual bool isAccepting() const; - virtual size_t bytesWanted(Range const) const; + virtual size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const; virtual void complete(); virtual store_client_t storeClientType() const; virtual char const *getSerialisedMetaData(); @@ -224,7 +224,7 @@ public: bool isEmpty () const {return true;} - virtual size_t bytesWanted(Range const aRange) const { return aRange.end; } + virtual size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const { return aRange.end; } void operator delete(void *address); void complete() {} diff --git a/src/store.cc b/src/store.cc index ea54b6cb0c..c3eafb8b32 100644 --- a/src/store.cc +++ b/src/store.cc @@ -257,7 +257,7 @@ StoreEntry::delayAwareRead(int fd, char *buf, int len, AsyncCall::Pointer callba } size_t -StoreEntry::bytesWanted (Range const aRange) const +StoreEntry::bytesWanted (Range const aRange, bool ignoreDelayPools) const { if (mem_obj == NULL) return aRange.end; @@ -268,14 +268,10 @@ StoreEntry::bytesWanted (Range const aRange) const #endif - /* Always read *something* here - we haven't got the header yet */ - if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT)) - return aRange.end; - if (!mem_obj->readAheadPolicyCanRead()) return 0; - return mem_obj->mostBytesWanted(aRange.end); + return mem_obj->mostBytesWanted(aRange.end, ignoreDelayPools); } bool diff --git a/src/tests/stub_MemObject.cc b/src/tests/stub_MemObject.cc index ff89a1aa59..ab2c451a2c 100644 --- a/src/tests/stub_MemObject.cc +++ b/src/tests/stub_MemObject.cc @@ -105,12 +105,7 @@ MemObject::~MemObject() fatal ("Not implemented"); } -int -MemObject::mostBytesWanted(int max) const -{ - fatal ("Not implemented"); - return -1; -} +int MemObject::mostBytesWanted(int max, bool ignoreDelayPools) const STUB_RETVAL(-1) #if DELAY_POOLS DelayId diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc index 80cd517ba5..e22af2ffcc 100644 --- a/src/tests/stub_store.cc +++ b/src/tests/stub_store.cc @@ -19,7 +19,7 @@ StoreEntry::StoreEntry(const char *url, const char *log_url) STUB HttpReply const *StoreEntry::getReply() const STUB_RETVAL(NULL) void StoreEntry::write(StoreIOBuffer) STUB bool StoreEntry::isAccepting() const STUB_RETVAL(false) -size_t StoreEntry::bytesWanted(Range const) const STUB_RETVAL(0) +size_t StoreEntry::bytesWanted(Range const, bool) const STUB_RETVAL(0) void StoreEntry::complete() STUB store_client_t StoreEntry::storeClientType() const STUB_RETVAL(STORE_NON_CLIENT) char const *StoreEntry::getSerialisedMetaData() STUB_RETVAL(NULL)