Alex Rousskov [Sun, 1 May 2016 21:37:52 +0000 (15:37 -0600)]
Accumulate fewer unknown-size responses to avoid overwhelming disks.
Start swapping out an unknown-size entry as soon as size-based cache_dir
selection is no longer affected by the entry growth. If the entry
eventually exceeds the selected cache_dir entry size limits, terminate
the swapout.
The following description assumes that Squid deals with a cachable
response that lacks a Content-Length header. These changes should not
affect other responses.
Prior to these changes, StoreEntry::mayStartSwapOut() delayed swapout
decision until the entire response was received or the already
accumulated portion of the response exceeded the [global] store entry
size limit, whichever came first. This logic protected Store from
entries with unknown sizes. AFAICT, that protection existed for two
reasons:
* Incorrect size-based cache_dir selection: When cache_dirs use
different min-size and/or max-size settings, Squid cannot tell which
cache_dir the unknown-size entry belongs to and, hence, may select the
wrong cache_dir.
* Disk bandwidth/space waste: If the growing entry exceeds all cache_dir
max-size limits, the swapout has to be aborted, resulting in waste of
previously spent resources (primarily disk bandwidth and space).
The cost of those protections include RAM waste (up to maximum cachable
object size for each of the concurrent unknown-size entry downloads) and
sudden disk overload (when the entire delayed entry is written to disk
in a large burst of write requests initiated from a tight doPages() loop
at the end of the swapout sequence when the entry size becomes known).
The latter cost is especially high because swapping the entire large
object out in one function call can easily overflow disker queues and/or
block Squid while the OS drains disk write buffers in an emergency mode.
FWIW, AFAICT, cache_dir selection protection was the only reason for
introducing response accumulation (trunk r4446). The RAM cost was
realized a year later (r4954), and the disk costs were realized during
this project.
This change reduces those costs by starting to swap out an unknown-size
entry ASAP, usually immediately. In most caching environments, most
large cachable entries should be cached. It is usually better to spend
[disk] resources gradually than to deal with sudden bursts [of disk
write requests]. Occasional jolts make high performance unsustainable.
This change does not affect size-based cache_dir selection: Squid still
delays swapout until future entry growth cannot affect that selection.
Fortunately, in most configurations, the correct selection can happen
immediately because cache_dirs lack explicit min-size and/or max-size
settings and simply rely on the *same-for-all* minimum_object_size and
maximum_object_size values.
We could make the trade-off between costly protections (i.e., accumulate
until the entry size is known) and occasional gradual resource waste
(i.e., start swapping out ASAP) configurable. However, I think it is
best to wait for the use case that requires such configuration and can
guide the design of those new configuration options.
Side changes:
* Honor forgotten minimum_object_size for cache_dirs without min-size in
Store::Disk::objectSizeIsAcceptable() and fix its initial value to
correctly detect a manually configured cache_dir min-size (which may
be zero). However, the fixed bug is probably hidden by another (yet
unfixed) bug: checkTooSmall() forgets about cache_dirs with min-size!
* Allow unknown-size objects into the shared memory cache, which code
could handle partial writes (since collapsed forwarding changes?).
* Fixed Rock::SwapDir::canStore() handling of unknown-size objects. I do
not see how such objects could get that far before, but if they could,
most would probably be cached because the bug would hide the unknown
size from Store::Disk::canStore() that declares them unstorable.
Cleanup: convert late initialized objects to MEMPROXY_CLASS
Convert all the objects using the libmem "old API" for as-needed pools
to using the MEMPROXY_CLASS() API which is better designed for late
initialization.
Alex Rousskov [Mon, 18 Apr 2016 19:08:25 +0000 (13:08 -0600)]
Stop parsing response prefix after discovering an "HTTP/0.9" response.
Otherwise, our "X-Transformed-From: HTTP/0.9" headers are going to
be ignored, and the rest of the received bytes are going to be parsed
(and modified!) as an HTTP/1 response header, followed by message body.
cachemgr.cgi: use dynamic MemBuf for internal content generation
Using a fixed size buffer limits how big content lines can be. Modern
HTTP is fast reaching the point where such limits are problematic.
Also fixes incorrect uses of snprintf() by removing them.
Some servers cause an SSL handshake error with peek and splice.
The problem is related to the TLS Session Tickets extension handling. Squid
expects always a Tls Session Tickets extension, included in server hello
message, to assume that the ticket accepted and the session is a resumed
session, which is not always true.
It did not catch all cases of the SP it was intended to and the chunked
encoding parse will need significantly different changes pending IETF WG
discussions.
Keep the violationLevel() member, which will be useful in general.
We should not expect the exact match because, as discovered during bug
3805 (r13947) fix, shared Segment::size() may exceed the originally
requested RAM amount due to stat() page rounding done by OSes like OS X.
Unfortunately, this rounding weakens the failed consistency check a lot.
TODO: Store the exact requested size and check against that as well.
AFAICT, the default copy constructor should work and the removed
explicit constructor was not copying the staticContext member, for no
documented reason (that I could find). It was also unused [in my tests].
If the partial copy constructor was abused for something useful, then a
different approach should be found -- the one that does not violate the
standard copy constructor post-conditions.
Squid crashes on startup when the parent process exit()s after fork()ing
the kid process. Squid may also crash on shutdown after exiting main().
In both cases, the crashes are build- and environment-specific. Many
environments show no problems at all. Even disabling compiler
optimizations may prevent crashes. When crashes do happen, their
symptoms (e.g., backtrace) point to problems during destruction of
global objects, but the details point to innocent objects (e.g., PortCfg
or SSL_CTX).
In some environments, the following malloc error is printed on console
before the crash: "corrupted double-linked list".
This change replaces two StatHist globals used for SBuf statistics
collection with always-available singletons. The replaced globals could
be destructed before the last SBuf object using them, leading to memory
corruption (that would eventually crash Squid).
It is possible that the connection will be closed somewhere inside
"clientTunnelOnError" call, inside ConnStateData::fakeAConnectRequest which
is called by ConnStateData::clientTunnelOnError or inside spliceOnError()
while trying to splice(). In this case the callers should be informed to abort
imediatelly, but instead continues, and try to set timeout handler on closed
connection.
This patch:
- Modify ConnStateData::fakeAConnectRequest and ConnStateData::splice methods to return boolean and false on error.
- Does not close the connection inside ConnStateData::fakeAConnectRequest but
instead return false and allow callers to close the connection if required.
Alex Rousskov [Thu, 7 Apr 2016 00:08:06 +0000 (18:08 -0600)]
Fixed NotNode (!acl) naming: Terminate the name before strncat(name).
The fix may reduce or even eliminate garbage in logged ACL names (at
least). The bug was exposed by valgrind's "Conditional jump or move
depends on uninitialised value(s)" error.
author: Nathan Hoad <nathan@getoffmalawn.com>
Add chained certificates and signing certificate to peek-then-bumped connections.
The scenario this patch addresses is when Squid is configured with an
intermediate signing CA certificate, and clients have the root CA installed on
their machines. What happens is that the generated certificates come down with
an unknown issuer (the intermediate signing certificate), with no
intermediates, so they are rejected. By adding the configured certificate chain
as old client-first mode did, the intermediate and root certificates come down
as well, resulting in the issuer being identified and the connection being
established "securely".
This work is submitted on behalf of Bloomberg L.P.
However, pinger only drops setuid/setgid, and won't drop capabilities
after sockets are opened (when it is setuid-root, setuid(getuid()) also
drops capabilities, no code changes necessary; however, if it is only
setcap'ed, setuid() is no-op).
Fix is minimally tested, seems to work fine with both/either `setcap`
and `chmod u+s`; non-linux/non-libcap configurations should not be
affected).
Nathan Hoad [Fri, 25 Mar 2016 13:03:30 +0000 (02:03 +1300)]
Fix memory leak of AccessLogentry::url
... created by ACLFilledChecklist::syncAle().
::syncAle() is the only place in the codebase that assigns a URL that
AccessLogEntry is expected to free(), which AccessLogEntry doesn't do.
This results in a memory leak.
Alex Rousskov [Thu, 24 Mar 2016 17:02:25 +0000 (11:02 -0600)]
Added shared_memory_locking configuration directive to control mlock(2).
Locking shared memory at startup avoids SIGBUS crashes when kernel runs
out of RAM during runtime. Why not enable it by default? Unfortunately,
locking requires privileges and/or much-higher-than-default
RLIMIT_MEMLOCK limits. Thus, requiring locked memory by default is
likely to cause too many complaints, especially since Squid has not
required that before. The default is off, at least for now.
As we gain more experience, we may try to enable locking by default
while making default locking failures non-fatal and warning about
significant [accumulated] locking delays.
Bug description:
- The client side and server side are finished
- On server side the Ftp::Relay::finalizeDataDownload() is called and
schedules the Ftp::Server::originDataCompletionCheckpoint
- On client side the "Ftp::Server::userDataCompletionCheckpoint" is
called. This is schedules a write to control connection and closes
data connection.
- The Ftp::Server::originDataCompletionCheckpoint is called which is
trying to write to control connection and the assertion triggered.
This bug is an corner case, where the client-side (FTP::Server) should
wait for the server side (Ftp::Client/Ftp::Relay) to finish its job before
respond to the FTP client. In this bug the existing mechanism, designed
to handle such problems, did not worked correctly and resulted to a double
write response to the client.
This patch try to fix the existing mechanism as follows:
- When Ftp::Server receives a "startWaitingForOrigin" callback, postpones
writting possible responses to the client and keeps waiting for the
stopWaitingForOrigin callback
- When the Ftp::Server receives a "stopWaitingForOrigin" callback,
resumes any postponed response.
- When the Ftp::Client starts working on a DATA-related transaction, calls the
Ftp::Server::startWaitingForOrigin callback
- When the Ftp::Client finishes its job or when its abort abnormaly, checks
whether it needs to call Ftp::Server::stopWaitingForOrigin callback.
- Also this patch try to fix the status code returned to the FTP client
taking in account the status code returned by FTP server. The
"Ftp::Server::stopWaitingForOrigin" is used to pass the returned status code
to the client side.
Author: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Added ACL-driven server_pconn_for_nonretriable squid.conf directive.
This directive provides fine-grained control over persistent connection
reuse when forwarding HTTP requests that Squid cannot retry. It is
useful in environments where opening new connections is very expensive
and race conditions associated with persistent connections are very rare
and/or only cause minor problems.
Alex Rousskov [Sat, 12 Mar 2016 18:40:29 +0000 (11:40 -0700)]
Trying to avoid "looser throw specifier" error with Wheezy GCC.
AFAICT, the default CbdataParent destructor gets implicit
"noexcept(true)" specifier (because the default destructor does not
throw itself, and CbdataParent has no data members or parents that could
have contributed potentially throwing destructors). The AsyncJob child
uses a lot of things that might throw during destruction (the compiler
cannot tell for sure because we do not use noexcept specifiers). Thus,
the compiler has to use "noexcept(false)" specifier for ~AsyncJob, which
is "looser" that "noexcept(true)" for ~CbdataParent and, hence, violates
the parent interface AsyncJob is implementing/overriding.
I have doubts about the above analysis because many other compilers,
including GCC v5 and clang are happy with the default virtual
CbdataParent destructor. If my analysis is correct, then the rule of
thumb is: Base classes must not use "= default" destructors until all
our implicit destructors become "noexcept".
AsyncJob classes can now use C++11 overrides as long as they use the new
CBDATA_CHILD() macro instead of old CBDATA_CLASS().
I have prohibited multiple CBDATA_CHILD() classes on the same
inheritance branch by adding the "final" specifier to toCbdata(). Such
classes feel dangerous because they may have different sizes and it is
not obvious to me whether the cbdata code will call the right size-
specific delete for them. We can easily relax this later if needed.
Alex Rousskov [Fri, 11 Mar 2016 18:00:51 +0000 (11:00 -0700)]
Bug 7: Update cached entries on 304 responses.
New Store API to update entry metadata and headers on 304s.
Support entry updates in shared memory cache and rock cache_dirs.
No changes to ufs-based cache_dirs: Their entries are still not updated.
* Atomic StoreEntry metadata updating
StoreEntry metadata (swap_file_sz, timestamps, etc.) is used
throughout Squid code. Metadata cannot be updated atomically because
it has many fields, but a partial update to those fields causes
assertions. Still, we must update metadata when updating HTTP
headers. Locking the entire entry for a rewrite does not work well
because concurrent requests will attempt to download a new entry
copy, defeating the very HTTP 304 optimization we want to support.
Ipc::StoreMap index now uses an extra level of indirection (the
StoreMap::fileNos index) which allows StoreMap control which
anchor/fileno is associated with a given StoreEntry key. The entry
updating code creates a disassociated (i.e., entry/key-less) anchor,
writes new metadata and headers using that new anchor, and then
_atomically_ switches the map to use that new anchor. This allows old
readers to continue reading using the stale anchor/fileno as if
nothing happened while a new reader gets the new anchor/fileno.
Shared memory usage increase: 8 additional bytes per cache entry: 4
for the extra level of indirection (StoreMapFileNos) plus 4 for
splicing fresh chain prefix with the stale chain suffix
(StoreMapAnchor::splicingPoint). However, if the updated headers are
larger than the stale ones, Squid will allocate shared memory pages
to accommodate for the increase, leading to shared memory
fragmentation/waste for small increases.
* Revamped rock index rebuild process
The index rebuild process had to be completely revamped because
splicing fresh and stale entry slot chain segments implies tolerating
multiple entry versions in a single chain and the old code was based
on the assumption that different slot versions are incompatible. We
were also uncomfortable with the old cavalier approach to accessing
two differently indexed layers of information (entry vs. slot) using
the same set of class fields, making it trivial to accidentally
access entry data while using slot index.
During the rewrite of the index rebuilding code, we also discovered a
way to significantly reduce RAM usage for the index build map (a
temporary object that is allocated in the beginning and freed at the
end of the index build process). The savings depend on the cache
size: A small cache saves about 30% (17 vs 24 bytes per entry/slot)
while a 1TB cache_dir with 32KB slots (which implies uneven
entry/slot indexes) saves more than 50% (~370MB vs. ~800MB).
Adjusted how invalid slots are counted. The code was sometimes
counting invalid entries and sometimes invalid entry slots. We should
always count _slots_ now because progress is measured in the number
of slots scanned, not entries loaded. This accounting change may
surprise users with much higher "Invalid entries" count in cache.log
upon startup, but at least the new reports are meaningful.
This rewrite does not attempt to solve all rock index build problems.
For example, the code still assumes that StoreEntry metadata fits a
single slot which is not always true for very small slots.
Alex Rousskov [Fri, 11 Mar 2016 17:24:13 +0000 (10:24 -0700)]
Removed SWAPOUT_WRITING assertion from storeSwapMetaBuild().
I do not see any strong dependency of that code on that state and we
need to be able to build swap metadata when updating a stale entry
(which would not normally be in the SWAPOUT_WRITING state).
The biggest danger is that somebody calls storeSwapMetaBuild() when the
entry metadata is not yet stable. I am not sure we have a way of
detecting that without using something as overly strong as
SWAPOUT_WRITING.