From: Amos Jeffries Date: Sun, 8 Feb 2009 09:34:00 +0000 (+1300) Subject: Author: Francesco Chemolli + Amos Jeffries X-Git-Tag: SQUID_3_0_STABLE14~31 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bfa461cd483a548aa7b98807e71ba6bcce91519d;p=thirdparty%2Fsquid.git Author: Francesco Chemolli + Amos Jeffries Pconn not being used when they should. A slight misalignment between the keys generated for push and pop of connections to the waiting pool caused new connections never to match any of the existing connections. This patch makes several alterations to achieve a fix: - reduces the FwdState push logics down into a simple selection in pconnPush function which previously was a dumb wrapper. - adds a dump of current hash keys to the cacheManager pconn report - adds much better debugging to the pconn process at level 48,3 and 48,6 - adds some additional documentation of code to the related call tree Pconn API after this patch : The Pconn KEY takes several parameters (host, port, domain, client-ip). For HTTP requests this is normally generated from the request data of same name with domain being optional since it may be identical to host. However for peer-sourced requests this alters slightly and the host:port fields become the peer NAME and HTTP-PORT. This means the pconn key in abstract becomes a key to the TCP remote-end of the link with an optional anchor on the domain being requested. --- diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 80a840dd8d..40b7292c24 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -267,7 +267,7 @@ HttpRequest::prefixLen() header.len + 2; } -/* +/** * Returns true if HTTP allows us to pass this header on. Does not * check anonymizer (aka header_access) configuration. */ diff --git a/src/forward.cc b/src/forward.cc index b8b08b1d7c..3a899f1aa1 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -767,7 +767,6 @@ FwdState::connectStart() FwdServer *fs = servers; const char *host; unsigned short port; - const char *domain = NULL; int ctimeout; int ftimeout = Config.Timeout.forward - (squid_curtime - start_t); #if LINUX_TPROXY @@ -784,16 +783,9 @@ FwdState::connectStart() debugs(17, 3, "fwdConnectStart: " << url); if (fs->_peer) { - host = fs->_peer->host; - port = fs->_peer->http_port; ctimeout = fs->_peer->connect_timeout > 0 ? fs->_peer->connect_timeout : Config.Timeout.peer_connect; - - if (fs->_peer->options.originserver) - domain = request->host; } else { - host = request->host; - port = request->port; ctimeout = Config.Timeout.connect; } @@ -809,7 +801,16 @@ FwdState::connectStart() if (ftimeout < ctimeout) ctimeout = ftimeout; - fd = fwdPconnPool->pop(host, port, domain, client_addr, checkRetriable()); + if(fs->_peer) { + host = fs->_peer->host; + port = fs->_peer->http_port; + fd = fwdPconnPool->pop(fs->_peer->name, fs->_peer->http_port, request->host, client_addr, checkRetriable()); + } + else { + host = request->host; + port = request->port; + fd = fwdPconnPool->pop(host, port, NULL, client_addr, checkRetriable()); + } if (fd >= 0) { debugs(17, 3, "fwdConnectStart: reusing pconn FD " << fd); server_fd = fd; @@ -1137,11 +1138,24 @@ FwdState::reforwardableStatus(http_status s) /* NOTREACHED */ } +/** + * Decide where details need to be gathered to correctly describe a persistent connection. + * What is needed: + * \item host name of server at other end of this link (either peer or requested host) + * \item port to which we connected the other end of this link (for peer or request) + * \item domain for which the connection is supposed to be used + * \item address of the client for which we made the connection + */ void - -FwdState::pconnPush(int fd, const char *host, int port, const char *domain, struct IN_ADDR *client_addr) +FwdState::pconnPush(int fd, const peer *_peer, const HttpRequest *req, const char *domain, struct in_addr *client_addr) { - fwdPconnPool->push(fd, host, port, domain, client_addr); + if (_peer) { + fwdPconnPool->push(fd, _peer->name, _peer->http_port, domain, client_addr); + } else { + /* small performance improvement, using NULL for domain instead of listing it twice */ + /* although this will leave a gap open for url-rewritten domains to share a link */ + fwdPconnPool->push(fd, req->host, req->port, NULL, client_addr); + } } void diff --git a/src/forward.h b/src/forward.h index ad9b0f15e7..d587c50ce0 100644 --- a/src/forward.h +++ b/src/forward.h @@ -5,6 +5,7 @@ class CacheManager; class ErrorState; +class HttpRequest; #include "comm.h" @@ -44,7 +45,7 @@ public: bool checkRetry(); bool checkRetriable(); void dispatch(); - void pconnPush(int fd, const char *host, int port, const char *domain, struct IN_ADDR *client_addr); + void pconnPush(int fd, const peer *_peer, const HttpRequest *req, const char *domain, struct in_addr *client_addr); bool dontRetry() { return flags.dont_retry; } diff --git a/src/http.cc b/src/http.cc index f11e7715e8..28e63877eb 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1167,6 +1167,7 @@ HttpStateData::processReplyBody() comm_remove_close_handler(fd, httpStateFree, this); fwd->unregister(fd); + #if LINUX_TPROXY if (orig_request->flags.tproxy) @@ -1174,14 +1175,7 @@ HttpStateData::processReplyBody() #endif - if (_peer) { - if (_peer->options.originserver) - fwd->pconnPush(fd, _peer->name, orig_request->port, orig_request->host, client_addr); - else - fwd->pconnPush(fd, _peer->name, _peer->http_port, NULL, client_addr); - } else { - fwd->pconnPush(fd, request->host, request->port, NULL, client_addr); - } + fwd->pconnPush(fd, _peer, request, orig_request->host, client_addr); fd = -1; diff --git a/src/pconn.cc b/src/pconn.cc index 16cd75db85..1412083a13 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -1,6 +1,5 @@ - /* - * $Id: pconn.cc,v 1.53.4.1 2008/02/24 12:06:41 amosjeffries Exp $ + * $Id$ * * DEBUG: section 48 Persistent Connections * AUTHOR: Duane Wessels @@ -178,17 +177,18 @@ const char * PconnPool::key(const char *host, u_short port, const char *domain, struct IN_ADDR *client_address) { - LOCAL_ARRAY(char, buf, SQUIDHOSTNAMELEN * 2 + 10); + LOCAL_ARRAY(char, buf, SQUIDHOSTNAMELEN * 3 + 10); if (domain && client_address) - snprintf(buf, SQUIDHOSTNAMELEN * 2 + 10, "%s:%d-%s/%s", host, (int) port, inet_ntoa(*client_address), domain); + snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d-%s/%s", host, (int) port, inet_ntoa(*client_address), domain); else if (domain && (!client_address)) - snprintf(buf, SQUIDHOSTNAMELEN * 2 + 10, "%s:%d/%s", host, (int) port, domain); + snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d/%s", host, (int) port, domain); else if ((!domain) && client_address) - snprintf(buf, SQUIDHOSTNAMELEN * 2 + 10, "%s:%d-%s", host, (int) port, inet_ntoa(*client_address)); + snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d-%s", host, (int) port, inet_ntoa(*client_address)); else - snprintf(buf, SQUIDHOSTNAMELEN * 2 + 10, "%s:%d", host, (int) port); + snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d", host, (int) port); + debugs(48,6,"PconnPool::key(" << host << "," << port << "," << domain << "," << inet_ntoa(*client_address) << "is {" << buf << "}" ); return buf; } @@ -212,6 +212,19 @@ PconnPool::dumpHist(StoreEntry * e) } } +void +PconnPool::dumpHash(StoreEntry *e) +{ + int i; + hash_link *walker = NULL; + hash_table *hid = table; + hash_first(hid); + + for (i = 0, walker = hid->next; walker; walker = hash_next(hid)) { + storeAppendPrintf(e, "\t item %5d: %s\n", i++, (char *)(walker->key)); + } +} + /* ========== PconnPool PUBLIC FUNCTIONS ============================================ */ PconnPool::PconnPool(const char *aDescr) : table(NULL), descr(aDescr) @@ -243,6 +256,7 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, stru } else if (shutting_down) { comm_close(fd); + debugs(48, 3, "PconnPool::push: Squid is shutting down. Refusing to do anything"); return; } @@ -253,8 +267,10 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, stru if (list == NULL) { list = new IdleConnList(aKey, this); - debugs(48, 3, "pconnNew: adding " << hashKeyStr(&list->hash)); + debugs(48, 3, "PconnPool::push: new IdleConnList for {" << hashKeyStr(&list->hash) << "}" ); hash_join(table, &list->hash); + } else { + debugs(48, 3, "PconnPool::push: found IdleConnList for {" << hashKeyStr(&list->hash) << "}" ); } list->push(fd); @@ -265,7 +281,7 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, stru debugs(48, 3, "PconnPool::push: pushed FD " << fd << " for " << aKey); } -/* +/** * Return a pconn fd for host:port if available and retriable. * Otherwise, return -1. * @@ -274,15 +290,17 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, stru * transactions create persistent connections but are not retriable. */ int - -PconnPool::pop(const char *host, u_short port, const char *domain, struct IN_ADDR *client_address, bool isRetriable) +PconnPool::pop(const char *host, u_short port, const char *domain, struct in_addr *client_address, bool isRetriable) { - IdleConnList *list; const char * aKey = key(host, port, domain, client_address); - list = (IdleConnList *)hash_lookup(table, aKey); - if (list == NULL) + IdleConnList *list = (IdleConnList *)hash_lookup(table, aKey); + if (list == NULL) { + debugs(48, 3, "PconnPool::pop: lookup for key {" << aKey << "} failed."); return -1; + } else { + debugs(48, 3, "PconnPool::pop: found " << hashKeyStr(&list->hash) << (isRetriable?"(to use)":"(to kill)") ); + } int fd = list->findUseableFD(); // search from the end. skip pending reads. @@ -361,7 +379,10 @@ PconnModule::dump(StoreEntry *e) int i; for (i = 0; i < poolCount; i++) { + storeAppendPrintf(e, "\n Pool %d Stats\n", i); (*(pools+i))->dumpHist(e); + storeAppendPrintf(e, "\n Pool %d Hash Table\n",i); + (*(pools+i))->dumpHash(e); } } diff --git a/src/pconn.h b/src/pconn.h index 8b08f07e82..98f7ae286a 100644 --- a/src/pconn.h +++ b/src/pconn.h @@ -52,6 +52,7 @@ public: int pop(const char *host, u_short port, const char *domain, struct IN_ADDR *client_address, bool retriable); void count(int uses); void dumpHist(StoreEntry *e); + void dumpHash(StoreEntry *e); void unlinkList(IdleConnList *list) const; private: