From: Eduard Bagdasaryan Date: Fri, 3 Mar 2017 22:15:10 +0000 (-0700) Subject: Fix two read-ahead problems related to delay pools (or lack of thereof). X-Git-Tag: M-staged-PR71~229 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f1ba1fba3f8da3a06496f200862b9201876ec013;p=thirdparty%2Fsquid.git Fix two read-ahead problems related to delay pools (or lack of thereof). 1. Honor EOF on Squid-to-server connections with full read ahead buffers and no clients when --enable-delay-pools is used without any delay pools configured in squid.conf. Since trunk r6150. Squid delays reading from the server after buffering read_ahead_gap bytes that are not yet sent to the client. A delayed read is normally resumed after Squid sends more buffered bytes to the client. See readAheadPolicyCanRead() and kickReads(). However, Squid was not resuming the delayed read after all Store clients were gone. If quick_abort prevents Squid from immediately closing the corresponding Squid-to-server connection, then the connection gets stuck until read_timeout (15m), even if the server closes much sooner, -- without reading from the server, Squid cannot detect the connection closure. The affected connections enter the CLOSE_WAIT state. Kicking delayed read when the last client leaves fixes the problem. The removal of any client, including the last one, may change readAheadPolicyCanRead() answer and, hence, deserves a kickReads() call. Why "without any delay pools configured in squid.conf"? When classic (i.e., delay_pool_*) delay pools are configured, Squid kicks all delayed reads every second. That periodic kicking is an old design bug, but it resumes stuck reads when all Store clients are gone. Without classic delay pools, there is no periodic kicking. This fix does not address that old bug but removes Squid hidden dependence on its side effect. Note that the Squid-to-server connections with full read-ahead buffers still remain "stuck" if there are non-reading clients. There is nothing Squid can do about them because we cannot reliably detect EOF without reading at least one byte and such reading is not allowed by the read ahead gap. In other words, non-reading clients still stall server connections. While fixing this, I moved all CheckQuickAbort() tests into CheckQuickAbortIsReasonable() because we need a boolean function to avoid kicking aborted entries and because the old separation was rather awkward -- CheckQuickAbort() contained "reasonable" tests that were not in CheckQuickAbortIsReasonable(). All the aborting tests and their order were preserved during this move. The moved tests gained debugging. According to the existing test order in CheckQuickAbortIsReasonable(), the above problem can be caused by: * non-private responses with a known content length * non-private responses with unknown content length, having quick_abort_min set to -1 KB. 2. Honor read_ahead_gap with --disable-delay-pools. Since trunk r13954. This fix also addresses "Perhaps these two calls should both live in MemObject" comment and eliminates existing code duplication. --- diff --git a/src/MemObject.cc b/src/MemObject.cc index 69961aa19d..67eb5ed3bc 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -437,6 +437,14 @@ MemObject::setNoDelay(bool const newValue) void MemObject::delayRead(DeferredRead const &aRead) { +#if USE_DELAY_POOLS + if (readAheadPolicyCanRead()) { + if (DelayId mostAllowedId = mostBytesAllowed()) { + mostAllowedId.delayRead(aRead); + return; + } + } +#endif deferredReads.delayRead(aRead); } diff --git a/src/http.cc b/src/http.cc index 7c1281c04f..10273c0d13 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1128,7 +1128,6 @@ HttpStateData::persistentConnStatus() const return statusIfComplete(); } -#if USE_DELAY_POOLS static void readDelayed(void *context, CommRead const &) { @@ -1136,7 +1135,6 @@ readDelayed(void *context, CommRead const &) state->flags.do_next_read = true; state->maybeReadVirginBody(); } -#endif void HttpStateData::readReply(const CommIoCbParams &io) @@ -1171,23 +1169,13 @@ HttpStateData::readReply(const CommIoCbParams &io) CommIoCbParams rd(this); // will be expanded with ReadNow results rd.conn = io.conn; rd.size = entry->bytesWanted(Range(0, inBuf.spaceSize())); -#if USE_DELAY_POOLS - if (rd.size < 1) { - assert(entry->mem_obj); - /* read ahead limit */ - /* Perhaps these two calls should both live in MemObject */ + if (rd.size <= 0) { + assert(entry->mem_obj); AsyncCall::Pointer nilCall; - if (!entry->mem_obj->readAheadPolicyCanRead()) { - entry->mem_obj->delayRead(DeferredRead(readDelayed, this, CommRead(io.conn, NULL, 0, nilCall))); - return; - } - - /* delay id limit */ - entry->mem_obj->mostBytesAllowed().delayRead(DeferredRead(readDelayed, this, CommRead(io.conn, NULL, 0, nilCall))); + entry->mem_obj->delayRead(DeferredRead(readDelayed, this, CommRead(io.conn, NULL, 0, nilCall))); return; } -#endif switch (Comm::ReadNow(rd, inBuf)) { case Comm::INPROGRESS: diff --git a/src/store.cc b/src/store.cc index d974882f3b..2220725be8 100644 --- a/src/store.cc +++ b/src/store.cc @@ -196,24 +196,10 @@ StoreEntry::delayAwareRead(const Comm::ConnectionPointer &conn, char *buf, int l * ->deferRead (fd, buf, len, callback, DelayAwareRead, this) */ - if (amountToRead == 0) { + if (amountToRead <= 0) { assert (mem_obj); - /* read ahead limit */ - /* Perhaps these two calls should both live in MemObject */ -#if USE_DELAY_POOLS - if (!mem_obj->readAheadPolicyCanRead()) { -#endif - mem_obj->delayRead(DeferredRead(DeferReader, this, CommRead(conn, buf, len, callback))); - return; -#if USE_DELAY_POOLS - } - - /* delay id limit */ - mem_obj->mostBytesAllowed().delayRead(DeferredRead(DeferReader, this, CommRead(conn, buf, len, callback))); + mem_obj->delayRead(DeferredRead(DeferReader, this, CommRead(conn, buf, len, callback))); return; - -#endif - } if (fd_table[conn->fd].closing()) { diff --git a/src/store_client.cc b/src/store_client.cc index afd95932d7..1d4b67eeca 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -41,7 +41,6 @@ static StoreIOState::STRCB storeClientReadHeader; static void storeClientCopy2(StoreEntry * e, store_client * sc); static EVH storeClientCopyEvent; static bool CheckQuickAbortIsReasonable(StoreEntry * entry); -static void CheckQuickAbort(StoreEntry * entry); CBDATA_CLASS_INIT(store_client); @@ -697,11 +696,11 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data) assert(e->locked()); // An entry locked by others may be unlocked (and destructed) by others, so - // we must lock again to safely dereference e after CheckQuickAbort(). + // we must lock again to safely dereference e after CheckQuickAbortIsReasonable(). e->lock("storeUnregister"); - if (mem->nclients == 0) - CheckQuickAbort(e); + if (CheckQuickAbortIsReasonable(e)) + e->abort(); else mem->kickReads(); @@ -760,9 +759,32 @@ storePendingNClients(const StoreEntry * e) static bool CheckQuickAbortIsReasonable(StoreEntry * entry) { + assert(entry); + debugs(90, 3, "entry=" << *entry); + + if (storePendingNClients(entry) > 0) { + debugs(90, 3, "quick-abort? NO storePendingNClients() > 0"); + return false; + } + + if (!shutting_down && Store::Root().transientReaders(*entry)) { + debugs(90, 3, "quick-abort? NO still have one or more transient readers"); + return false; + } + + if (entry->store_status != STORE_PENDING) { + debugs(90, 3, "quick-abort? NO store_status != STORE_PENDING"); + return false; + } + + if (EBIT_TEST(entry->flags, ENTRY_SPECIAL)) { + debugs(90, 3, "quick-abort? NO ENTRY_SPECIAL"); + return false; + } + MemObject * const mem = entry->mem_obj; assert(mem); - debugs(90, 3, "entry=" << entry << ", mem=" << mem); + debugs(90, 3, "mem=" << mem); if (mem->request && !mem->request->flags.cachable) { debugs(90, 3, "quick-abort? YES !mem->request->flags.cachable"); @@ -824,31 +846,6 @@ CheckQuickAbortIsReasonable(StoreEntry * entry) return true; } -/// Aborts a swapping-out entry if nobody needs it any more _and_ -/// continuing swap out is not reasonable per CheckQuickAbortIsReasonable(). -static void -CheckQuickAbort(StoreEntry * entry) -{ - assert (entry); - - if (storePendingNClients(entry) > 0) - return; - - if (!shutting_down && Store::Root().transientReaders(*entry)) - return; - - if (entry->store_status != STORE_PENDING) - return; - - if (EBIT_TEST(entry->flags, ENTRY_SPECIAL)) - return; - - if (!CheckQuickAbortIsReasonable(entry)) - return; - - entry->abort(); -} - void store_client::dumpStats(MemBuf * output, int clientNumber) const {