Re: [RFC] connections logging
On 24/06/2014 12:26 p.m., Alex Rousskov wrote: On 06/20/2014 03:04 AM, Amos Jeffries wrote: I am playing with the idea of adding a new log file to record just the connections handled by Squid (rather than the HTTP request/reply transactions). This log would include connections opened but never used, port details, connection lifetimes, re-use counts on persistent connections, SSL/TLS presence/version (or not), and other connection details we find a need for later. The driver behind this is to help resolve a growing amount of user queries regarding happy eyeballs idle connections and broken TLS connections. We are also adding other potentially never-used connections ourselves with the standby pools. A couple of days ago, I came across another use case for logging connections unrelated to Happy Eyeballs. It sounds like this is going to be a generally useful feature. My use case is still being clarified, but here are the already known non-obvious requirements: 1. Logging when the connection is received (unlike transactions that are logged when the transaction is completed). 2. Logging client-to-Squid connections. 3. Introduction of some kind of unique connection ID that can be associated with HTTP/etc transactions on that connection, correlating access.log and connection.log entries. [ uniqueness scope is being clarified, but I hope it would just be a single worker lifetime ] For IDs I was thinking of just emitting the MasterXaction::id value sent in by TcpAcceptor. Assumign this is defined as the TCP transaction (see below). Let's coordinate our efforts! Sure. The Squid native access.log appears unsuitable for adjustment given its widely used format and the number of details to be logged. Agreed. We should support a similarly configurable log format and other existing logging knobs but use a different, dedicated log (or logs if we want to record from-Squid connections and to-Squid connections differently). Reusable logformat codes such as %la and %lp should be reused, but new ones will probably be needed as well. Ack. There is also a useful side effect of MasterXaction. At present the TcpAcceptor generates a MasterXaction object to record initial connection details for the handler, this creates a bit of complexity in the recipient regarding whether the MasterXactino object applies to a future HTTP request or one which is currently being parsed. This could be simplified if the connection was a transaction and the HTTP requests on it each a different transaction spawned from the connection MasterXaction when parsing a new requst. I am not sure what you mean by if the connection was a transaction. I mean defining the TCP actions of opening a socket/connnection, its entire lifetime, and close() as being one transaction. With the TcpAcceptor produced MasterXaction representing that cycle. HTTP requests arriving on the connection being separate transactions spawned by the parsing actions. (unfortunately the read(2) does not always nicely match a parse event identifying the exact HTTP request start). So we have two new transaction types there. TCP transaction and HTTP transaction, to go alongside ICAP and eCAP transaction etc. The MasterXaction is usually associated with at least one connection (via the small t transaction using that connection), and one connection is usually associated with at least one MasterTransaction, but transactions and connections are completely different concepts at Squid level IMO. I doubt we should try to merge the concept of a transaction and a connection somehow. Generating the HTTP transaction MasterXaction object (child of MasterXaction?) by cloning it from the TCP connection one represents that relationship. The clone could be seeded with the parents id value to persist that info for later use (eg your start of HTTP transaction log entry). MasterTransaction is created at connection acceptance time because that is when the first HTTP/etc. transaction starts (parsing headers is not the first step) and, hence, that is when the master transaction starts. Hmm. We have some issue here satisfying the people who want HTTP transation only to represent the time of request+reply versus browser happy eyeballs (and now Squid) opening standby connections with long durations before first byte. That is one reason supporting separate transaction MasterXaction for TCP and HTTP. We could make the connection acceptance event itself a transaction (an object of some new AcceptXaction class inherited from a generic Transaction class and associated with the corresponding Master Transaction object just like HTTP and ICAP transactions should be). I do not yet see what that would actually buy us except more classes to worry about, but, at the very least, it would not be conceptually wrong. I agree accept by itself is probably useless. The longer TCP lifecycle on the other hand (see above) has some use for
Re: [RFC] connections logging
On 24/06/2014 4:41 p.m., Kinkie wrote: On Tue, Jun 24, 2014 at 2:26 AM, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/20/2014 03:04 AM, Amos Jeffries wrote: I am playing with the idea of adding a new log file to record just the connections handled by Squid (rather than the HTTP request/reply transactions). This log would include connections opened but never used, port details, connection lifetimes, re-use counts on persistent connections, SSL/TLS presence/version (or not), and other connection details we find a need for later. The driver behind this is to help resolve a growing amount of user queries regarding happy eyeballs idle connections and broken TLS connections. We are also adding other potentially never-used connections ourselves with the standby pools. A couple of days ago, I came across another use case for logging connections unrelated to Happy Eyeballs. It sounds like this is going to be a generally useful feature. My use case is still being clarified, but here are the already known non-obvious requirements: I have an use case, as well: timing logging. It would be useful to have a chance to log the timing of certain key moments in a request's processing path. Things like accept, end of slow acl matching, end of dns resolution(s), connected to uplink, and so on. This could help administrators identify congestion issues in their infrastructure. Kinkie And that use case gives us a good potential name for it. event.log ? Amos
Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID
On 06/24/2014 08:21 AM, Amos Jeffries wrote:
On 24/06/2014 1:44 a.m., Tsantilas Christos wrote:
On 06/23/2014 09:50 AM, Amos Jeffries wrote:
On 23/06/2014 5:43 a.m., Tsantilas Christos wrote:
in src/Notes.cc:
* replaceOrAdd iterates over the notes list twice in the event of
removals (due to remove() iterating over it from scratch). Please fix
this to iterate over the list only once and do an in-place replace if
the key is found already there, add if end() is reached.
* remove() appears to be unnecessary after the above correction.
I can fix a litle replaceOrAdd, but the remove() method still is needed.
By what code? I see it only being used by replaceOrAdd(), and only to
perform the unnecessary loop. It is fine as a remove() implementation,
but to me looks needless at this time.
Which loop is unnecessary?
You need one loop to iterate over src NotePairs and one loop to iterate
over this-entries to find and remove the existing item. The second loop
done by remove() method.
Am I missing something?
The replaceOrAdd() method method will be:
void
NotePairs::replaceOrAdd(const NotePairs *src)
{
for (std::vectorNotePairs::Entry *::const_iterator i =
src-entries.begin(); i != src-entries.end(); ++i) {
remove((*i)-name.termedBuf());
entries.push_back(new NotePairs::Entry((*i)-name.termedBuf(),
(*i)-value.termedBuf()));
}
}
in Notes.*:
* findFirst() no longer makes sense. There being no second now.
- Please rename to find().
This is not true. We can still support multiple values per key name.
I am suggesting to not touch this one.
Every helper callback is now using Update*() which uses replaceOrAddd()
to ensure uniqueness.
Not exactly uniqueness, just that the new key/value pairs will replace
any existing.
I see that the list output directly from the helper may contain
duplicates and might be used by the handler instead of the HttpRequest
merged list. We may want to change that to make handler behaviour match
behaviour of the ACL testing the HttpRequest list.
My understanding from discussion is that in future we may want to
support multiple values per key note. Maybe using a configuration
parameter, or other mechanism.
Moreover the helper may return for the same key multiple values:
... key=value1 key=value2 key=value3
Just if already exist the key 'key' in notes list will be replaced by
the new key/values pairs.
We can change the above and support only one value per key. My opinion
is that there is not any strong reason for denying multiple values per
key. It may be useful feature.
But I have not strong opinion on this.
in src/client_side_request.cc:
* ClientRequestContext::clientRedirectDone was previously calling
SyncNotes to ensure the AccessLogEntry and HttpRequest shared a notes
list before updating the request. Order is important here because Sync
will kill Squid with an assert if the objects have different notes list
objects.
Until that is fixed please retain the order of:
- sync first, then update the request.
* same in ClientRequestContext::clientStoreIdDone
It is the same, it is not make difference.
UpdateRequestNotes will just copy helper notes to the request notes. The
SyncNotes will check if AccessLogEntry and HttpRequest object points to
the same NotePairs object.
If you are going to retain it the way it is please add a note to the
AccessLogEntry::notes member indicating that it must NEVER be set other
than by calling SyncNotes() with the current transactions HttpRequest
object. Doing so will guarantee crashes after this patch goes in.
The AccessLogEntry::notes must never set. This is does not have to do
with the order the UpdataRequestNotes and SyncNotes called. The squid
will crash even if we call SyncNotes before UpdateRequestNotes.
However I have not any problem to change the order. Maybe make more
sense and also I can remove the if (!request.notes) block from
UpdateRequestNotes.
The burden will also be placed on auditors to check this with every
notes related patch. (I would rather avoid that extra work TBH).
OK, if I will avoid the extra work that way I will call SyncNotes before
UpdateRequestNotes :-)
Regards,
Christos
Amos
Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID
On 06/23/2014 11:00 PM, Amos Jeffries wrote:
On 24/06/2014 8:19 a.m., Alex Rousskov wrote:
On 06/23/2014 07:44 AM, Tsantilas Christos wrote:
On 06/23/2014 09:50 AM, Amos Jeffries wrote:
On 23/06/2014 5:43 a.m., Tsantilas Christos wrote:
clt_conn_id=ID
Associates the received ID with the client TCP connection.
The clt_conn_id=ID pair is treated as a regular annotation but
it persists across all transactions on the client connection
rather than disappearing after the current request. A helper
may update the client connection ID value during subsequent
requests by returning a new ID value. To send the connection
ID to the URL rewriter, use url_rewrite_extras:
url_rewrite_extras clt_conn_id=%{clt_conn_id}note ...
* can we call this client-tag= or connection-tag= or
connection-label=. We have ID used internally via InstanceId
template members, which are very different to this ID being
user-assigned.
The clt_conn_id is not a client tag, but it can be a connection-tag or
connection-label.
client-tag does not work well indeed because the tag is specific to
the connection, not the client, for some popular definitions of a
client. For example, many differently authenticated clients may
share the same tagged connection (when that connection comes from a peer
that aggregates traffic from many end-users).
My understanding was that a major use-case for this feature was
for faking various forms of
connection-based authentication when standard HTTP authentication
cannot be used.
Correct. There is a certain danger in conflating a use case with a
feature that resulted from that use case, but you have stated the
original motivation correctly.
Allowing client A to login client B, C, ...Z etc only makes sense if
the client and user are detached concepts at this level. Client
being strictly the client-server model type of client for whom any
number of users may exist, or proxying for other further away clients
in a multi-hop setup.
In the scope of this particular feature, only the former interpretation
of client applies: The agent making a connection to Squid. Distant
users indirectly delivered or proxied to Squid do not apply.
Either way by using this tag the helper is assuming that it is relevant
to all future requests on the whole connection. ie that there is either
one client, or one user which the note represents.
Not quite: One user or client may open multiple connections to Squid,
each possibly having its own ID/tag/label. The feature is about [client]
connection tagging, not client or user tagging.
Amos, does clt_conn_tag, clt_conn_mark, clt_conn_label, or clt_conn_note
(or their longer, partially spelled out versions) sound better than
clt_conn_id to you?
Yes. Any of them are better. tag has existing history from
exetrnal_acl_type interface that may want to be considered.
tag usage in external_acl_type is semantically similar to what we are
doing here. Unfortunately, tag is not called clt_request_tag, and it
has a different never-reset semantics.
mark is used in QoS (TOS/DSCP) connection marking context, a
semantically related concept. Note that some supported connection
markings can be reset/changed during a single connection lifetime
(AFAICT), just like the connection ID being proposed. On the other hand,
those markings alter bytes on the wire while the proposed feature does
not do that [directly].
note is used for various annotations with a different multi-value
semantics (unless we change that ASAP). Also, the proposed feature is
setting the _value_ of that annotation without controlling the note key
so it is somewhat wrong to call it clt_conn_note. It should be
clt_conn_note_value instead, which is probably too long/awkward.
None of these suffixes are a perfect fit. I do not have a strong
preference, but can suggest clt_conn_tag if nobody has a strong
preference or better ideas.
HTH,
Alex.
Re: [RFC] connections logging
On 06/24/2014 12:55 AM, Amos Jeffries wrote: On 24/06/2014 4:41 p.m., Kinkie wrote: On Tue, Jun 24, 2014 at 2:26 AM, Alex Rousskov wrote: On 06/20/2014 03:04 AM, Amos Jeffries wrote: The driver behind this is to help resolve a growing amount of user queries regarding happy eyeballs idle connections and broken TLS connections. We are also adding other potentially never-used connections ourselves with the standby pools. A couple of days ago, I came across another use case for logging connections unrelated to Happy Eyeballs. It sounds like this is going to be a generally useful feature. My use case is still being clarified, but here are the already known non-obvious requirements: I have an use case, as well: timing logging. It would be useful to have a chance to log the timing of certain key moments in a request's processing path. Things like accept, end of slow acl matching, end of dns resolution(s), connected to uplink, and so on. This could help administrators identify congestion issues in their infrastructure. And that use case gives us a good potential name for it. event.log ? I was also going to say that if we want to log more than just connection-specific events, then we should expand the proposal to accommodate arbitrary events. Needless to say, the initial implementation may only support one or two event kinds, but the design should allow for other future event kinds. Other known use cases include logging initial HIT or MISS detection: I had to patch Squid recently to enable that event logging (for legacy reasons the admin could not use ToS marks). This kind of circles back to the debugging discussion we are avoiding on another thread -- in both cases, we need an internal API to signal useful events and the corresponding configuration knobs to control their logging. The line between developer-useful (i.e., debugging) events and admin-useful events is blurry, but we do not have to define it if we just focus on logging admin-useful stuff. I would not be surprised if this eventually morphs into a better debugging interface as well. Cheers, Alex.
Re: [RFC] connections logging
On 06/24/2014 12:33 AM, Amos Jeffries wrote: On 24/06/2014 12:26 p.m., Alex Rousskov wrote: On 06/20/2014 03:04 AM, Amos Jeffries wrote: I am playing with the idea of adding a new log file to record just the connections handled by Squid (rather than the HTTP request/reply transactions). This log would include connections opened but never used, port details, connection lifetimes, re-use counts on persistent connections, SSL/TLS presence/version (or not), and other connection details we find a need for later. The driver behind this is to help resolve a growing amount of user queries regarding happy eyeballs idle connections and broken TLS connections. We are also adding other potentially never-used connections ourselves with the standby pools. A couple of days ago, I came across another use case for logging connections unrelated to Happy Eyeballs. It sounds like this is going to be a generally useful feature. My use case is still being clarified, but here are the already known non-obvious requirements: 1. Logging when the connection is received (unlike transactions that are logged when the transaction is completed). 2. Logging client-to-Squid connections. 3. Introduction of some kind of unique connection ID that can be associated with HTTP/etc transactions on that connection, correlating access.log and connection.log entries. [ uniqueness scope is being clarified, but I hope it would just be a single worker lifetime ] For IDs I was thinking of just emitting the MasterXaction::id value sent in by TcpAcceptor. Assumign this is defined as the TCP transaction (see below). Yes, MasterXaction ID can be copied to be used as a connection ID, but there is no good reason for such a reuse IMO. We need a Connection ID. IMO, it would be better to just add a Connection-dedicated InstanceId (especially if it satisfies the yet-unknown uniqueness scope requirements). Yes, an InstanceId-based ID would work for the worker lifetime uniqueness scope, and I hope that is all we need to support. I am not sure what you mean by if the connection was a transaction. I mean defining the TCP actions of opening a socket/connnection, its entire lifetime, and close() as being one transaction. With the TcpAcceptor produced MasterXaction representing that cycle. I strongly disagree that we should change the MasterXaction definition like that. We already have a Connection class to represent what you want, I think. Why drag Master Transaction into this, abandoning its current role of aggregating information about transactions caused by a single HTTP request (or equivalent)? HTTP transactions need a Master Transaction to correlate an HTTP request received from a client with the corresponding HTTP request(s) sent to the server, with the corresponding HTTP response(s) received from the server, with the corresponding ICAP messages, DNS activity, etc. A Master Transaction usually relates to several Connections as well, but Connection is not a Master Transaction. So we have two new transaction types there. TCP transaction and HTTP transaction, to go alongside ICAP and eCAP transaction etc. It is a bad idea to use the same terminology and class hierarchy for low-level transport concepts such as TCP and for high level HTTP transactions executed on top of a transport connection. We should just use Connection instead of TCP transaction. If we need TcpConnection, we can add that. The Connection hierarchy is separate from the Xaction hierarchy, but Xactions may use Connections, of course. The MasterXaction is usually associated with at least one connection (via the small t transaction using that connection), and one connection is usually associated with at least one MasterTransaction, but transactions and connections are completely different concepts at Squid level IMO. I doubt we should try to merge the concept of a transaction and a connection somehow. Generating the HTTP transaction MasterXaction object (child of MasterXaction?) by cloning it from the TCP connection one represents that relationship. The clone could be seeded with the parents id value to persist that info for later use (eg your start of HTTP transaction log entry). I am afraid we have a completely different view of what MasterXaction is and what protocol transactions are (or should be). It is possible that I am completely misinterpreting your statement about cloning and seeding, but if you are calling for creation of some abstract MasterXaction class that will be a parent for various specific FooMasterXaction classes such as HttpMasterXaction and TcpMasterXaction, then I see no need for (and a lot of harm from) that level of complexity. I do not know how to avoid another prolonged discussion here, but perhaps we can start with definitions of the proposed classes (instead of discussing cloning and seeding which are just low-level details)? The classes I am talking are, roughly: * MasterXaction: [Historical/aggregated
Re: [PATCH] Fixes to get more collapsed forwarding hits
On 06/23/2014 11:58 PM, Amos Jeffries wrote: On 24/06/2014 4:07 p.m., Alex Rousskov wrote: * Do not abandon writing a collapsed cache entry when we cannot cache the entry in RAM if the entry can be cached on disk instead. Both shared memory cache and the disk cache have to refuse to cache the entry for it to become non-collapsible. This dual refusal is difficult to detect because each cache may make the caching decision at different times. Added StoreEntry methods to track those decisions and react to them. Hmm. FTR: IMHO the only reasons a response should be non-collapsible is if it is a) private, b) authenticated to another user, c) explicitly known non-cacheable, d) a non-matching variant of the URL, or e) we already discarded some body bytes. Yes, there are many only reasons why a we may not be able to collapse what looked like a collapsible transaction at the beginning. The bullet we are discussing fixes a case where the Squid shared memory caching code said if we cannot cache this entry in the shared memory cache, then we cannot collapse onto this entry. That was wrong; we should still be able to collapse if the disk cache stores the entry. The factor of memory/disk caches not having provided cacheable size allow/reject result yet is no reason to be non-collapsible. It is a reason to wait on that result to determine the cacheability condition (c). Yes, kind of. Today, Squid treats HTTP cachable and storable in a Squid cache as two rather different things. The two conditions are checked and managed in different code places and at different times (with some overlap). And some of those decisions are unrelated to the entry size. However, they are essentially the same from user-perceived caching point of view and can be listed under a single explicitly known non-cacheable bullet if one is careful to interpret that correctly. +1. Although I do not know enough about store yet to do much in-depth audit of the logic changes. FWIW, knowing enough about Store makes me even less confident about changes like these ones. This is very tricky stuff not just because of the complexity of the features involved (caching, collapsing, etc.) but because there are very few simple invariants in Store. I rarely know for sure what I am going to break when changing this stuff. The code is getting better, and I am trying to document the invariants I find, but it is a very slow process, and there will be many more bugs, possibly from this patch as well. Cheers, Alex.
Re: [PATCH] Fixes to get more collapsed forwarding hits
We can't know except by hammering it. Did you run coadvisor and polygraph against it? If not, and if the branch is public, it's trivial tor un them against it now (thanks TMF, thanks Pavel!). It doesn't guarantee to catch all cases, but at least it should raise confidence. On Tue, Jun 24, 2014 at 7:37 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/23/2014 11:58 PM, Amos Jeffries wrote: On 24/06/2014 4:07 p.m., Alex Rousskov wrote: * Do not abandon writing a collapsed cache entry when we cannot cache the entry in RAM if the entry can be cached on disk instead. Both shared memory cache and the disk cache have to refuse to cache the entry for it to become non-collapsible. This dual refusal is difficult to detect because each cache may make the caching decision at different times. Added StoreEntry methods to track those decisions and react to them. Hmm. FTR: IMHO the only reasons a response should be non-collapsible is if it is a) private, b) authenticated to another user, c) explicitly known non-cacheable, d) a non-matching variant of the URL, or e) we already discarded some body bytes. Yes, there are many only reasons why a we may not be able to collapse what looked like a collapsible transaction at the beginning. The bullet we are discussing fixes a case where the Squid shared memory caching code said if we cannot cache this entry in the shared memory cache, then we cannot collapse onto this entry. That was wrong; we should still be able to collapse if the disk cache stores the entry. The factor of memory/disk caches not having provided cacheable size allow/reject result yet is no reason to be non-collapsible. It is a reason to wait on that result to determine the cacheability condition (c). Yes, kind of. Today, Squid treats HTTP cachable and storable in a Squid cache as two rather different things. The two conditions are checked and managed in different code places and at different times (with some overlap). And some of those decisions are unrelated to the entry size. However, they are essentially the same from user-perceived caching point of view and can be listed under a single explicitly known non-cacheable bullet if one is careful to interpret that correctly. +1. Although I do not know enough about store yet to do much in-depth audit of the logic changes. FWIW, knowing enough about Store makes me even less confident about changes like these ones. This is very tricky stuff not just because of the complexity of the features involved (caching, collapsing, etc.) but because there are very few simple invariants in Store. I rarely know for sure what I am going to break when changing this stuff. The code is getting better, and I am trying to document the invariants I find, but it is a very slow process, and there will be many more bugs, possibly from this patch as well. Cheers, Alex. -- Francesco
Re: [PATCH] Fixes to get more collapsed forwarding hits
On 06/24/2014 12:55 PM, Kinkie wrote: Did you run coadvisor and polygraph against it? Co-Advisor does not have collapsed forwarding cases (there are no explicit RFC 2616 MUSTs that cover CF although some cases can be certainly added to test some MUSTs in a CF context). Polygraph can be used to simulate CF-heavy environment, but all you will get from this patch is an improved hit ratio. It is not going to tell you which collapsing cases Squid have missed. If not, and if the branch is public, it's trivial tor un them against it now (thanks TMF, thanks Pavel!). It doesn't guarantee to catch all cases, but at least it should raise confidence. There is currently no branch for this -- just a trunk patch. A bigger problem is that you will need a different Polygraph workload -- the one we use for Squid regression testing via Jenkins is unlikely to produce a lot of CF opportunities. Overall, while I would certainly welcome Co-Advisor and Polygraph results, I decided to rely on existing trunk regression testing and not spend more time on organizing custom pre-commit tests. If somebody wants to volunteer to test this, please let me know. Thank you, Alex. On Tue, Jun 24, 2014 at 7:37 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/23/2014 11:58 PM, Amos Jeffries wrote: On 24/06/2014 4:07 p.m., Alex Rousskov wrote: * Do not abandon writing a collapsed cache entry when we cannot cache the entry in RAM if the entry can be cached on disk instead. Both shared memory cache and the disk cache have to refuse to cache the entry for it to become non-collapsible. This dual refusal is difficult to detect because each cache may make the caching decision at different times. Added StoreEntry methods to track those decisions and react to them. Hmm. FTR: IMHO the only reasons a response should be non-collapsible is if it is a) private, b) authenticated to another user, c) explicitly known non-cacheable, d) a non-matching variant of the URL, or e) we already discarded some body bytes. Yes, there are many only reasons why a we may not be able to collapse what looked like a collapsible transaction at the beginning. The bullet we are discussing fixes a case where the Squid shared memory caching code said if we cannot cache this entry in the shared memory cache, then we cannot collapse onto this entry. That was wrong; we should still be able to collapse if the disk cache stores the entry. The factor of memory/disk caches not having provided cacheable size allow/reject result yet is no reason to be non-collapsible. It is a reason to wait on that result to determine the cacheability condition (c). Yes, kind of. Today, Squid treats HTTP cachable and storable in a Squid cache as two rather different things. The two conditions are checked and managed in different code places and at different times (with some overlap). And some of those decisions are unrelated to the entry size. However, they are essentially the same from user-perceived caching point of view and can be listed under a single explicitly known non-cacheable bullet if one is careful to interpret that correctly. +1. Although I do not know enough about store yet to do much in-depth audit of the logic changes. FWIW, knowing enough about Store makes me even less confident about changes like these ones. This is very tricky stuff not just because of the complexity of the features involved (caching, collapsing, etc.) but because there are very few simple invariants in Store. I rarely know for sure what I am going to break when changing this stuff. The code is getting better, and I am trying to document the invariants I find, but it is a very slow process, and there will be many more bugs, possibly from this patch as well. Cheers, Alex.
Re: [PATCH] Fixes to get more collapsed forwarding hits
Amos already +1-ed the patch, I have no insights to add so unless someone speaks up real fast, we proceed. On Tue, Jun 24, 2014 at 9:29 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/24/2014 12:55 PM, Kinkie wrote: Did you run coadvisor and polygraph against it? Co-Advisor does not have collapsed forwarding cases (there are no explicit RFC 2616 MUSTs that cover CF although some cases can be certainly added to test some MUSTs in a CF context). Polygraph can be used to simulate CF-heavy environment, but all you will get from this patch is an improved hit ratio. It is not going to tell you which collapsing cases Squid have missed. If not, and if the branch is public, it's trivial tor un them against it now (thanks TMF, thanks Pavel!). It doesn't guarantee to catch all cases, but at least it should raise confidence. There is currently no branch for this -- just a trunk patch. A bigger problem is that you will need a different Polygraph workload -- the one we use for Squid regression testing via Jenkins is unlikely to produce a lot of CF opportunities. Overall, while I would certainly welcome Co-Advisor and Polygraph results, I decided to rely on existing trunk regression testing and not spend more time on organizing custom pre-commit tests. If somebody wants to volunteer to test this, please let me know. Thank you, Alex. On Tue, Jun 24, 2014 at 7:37 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/23/2014 11:58 PM, Amos Jeffries wrote: On 24/06/2014 4:07 p.m., Alex Rousskov wrote: * Do not abandon writing a collapsed cache entry when we cannot cache the entry in RAM if the entry can be cached on disk instead. Both shared memory cache and the disk cache have to refuse to cache the entry for it to become non-collapsible. This dual refusal is difficult to detect because each cache may make the caching decision at different times. Added StoreEntry methods to track those decisions and react to them. Hmm. FTR: IMHO the only reasons a response should be non-collapsible is if it is a) private, b) authenticated to another user, c) explicitly known non-cacheable, d) a non-matching variant of the URL, or e) we already discarded some body bytes. Yes, there are many only reasons why a we may not be able to collapse what looked like a collapsible transaction at the beginning. The bullet we are discussing fixes a case where the Squid shared memory caching code said if we cannot cache this entry in the shared memory cache, then we cannot collapse onto this entry. That was wrong; we should still be able to collapse if the disk cache stores the entry. The factor of memory/disk caches not having provided cacheable size allow/reject result yet is no reason to be non-collapsible. It is a reason to wait on that result to determine the cacheability condition (c). Yes, kind of. Today, Squid treats HTTP cachable and storable in a Squid cache as two rather different things. The two conditions are checked and managed in different code places and at different times (with some overlap). And some of those decisions are unrelated to the entry size. However, they are essentially the same from user-perceived caching point of view and can be listed under a single explicitly known non-cacheable bullet if one is careful to interpret that correctly. +1. Although I do not know enough about store yet to do much in-depth audit of the logic changes. FWIW, knowing enough about Store makes me even less confident about changes like these ones. This is very tricky stuff not just because of the complexity of the features involved (caching, collapsing, etc.) but because there are very few simple invariants in Store. I rarely know for sure what I am going to break when changing this stuff. The code is getting better, and I am trying to document the invariants I find, but it is a very slow process, and there will be many more bugs, possibly from this patch as well. Cheers, Alex. -- Francesco
Re: [PATCH] rock fixes and improvements
On 04/29/2014 05:29 AM, Amos Jeffries wrote: +1. All good apart form one minor nit: src/tests/stub_store.cc * the new checkCacheable() needs to be STUB_RETVAL(false). That can be added on merge. Finally merged as trunk r13475. Please note that another, unaudited fix for a frequent shared memory cache assertion was included in this merge: Avoid store_client.cc entry-swap_filen -1 || entry-swappingOut() asserts. A client may hit on an incomplete shared memory cache entry. Such entry is fully backed by the shared memory cache, but the copy of its data in local RAM may be trimmed. When that trimMemory() happens, StoreEntry::storeClientType() assumes DISK_CLIENT due to positive inmem_lo, and the store_client constructor asserts upon discovering that there is no disk backing. To improve shared cache effectiveness for being cached entries, we need to prevent local memory trimming while the shared cache entry is being filled (possibly by another worker, so this is far from trivial!) or, better, stop using the local memory for entries feeding off the shared memory cache. The latter would also require revising DISK_CLIENT designation to include entries backed by a shared memory cache. This production-tested fix essentially reorders two checks in StoreEntry::validToSend(): if (!mem_obj) // not backed by a memory cache and not collapsed return 0; -if (mem_obj-memCache.index = 0) // backed by a shared memory cache -return 1; - // StoreEntry::storeClientType() assumes DISK_CLIENT here, but there is no -// disk cache backing so we should not rely on the store cache at all. This -// is wrong for range requests that could feed off nibbled memory (XXX). -if (mem_obj-inmem_lo) // in local memory cache, but got nibbled at +// disk cache backing that store_client constructor will assert. XXX: This +// is wrong for range requests (that could feed off nibbled memory) and for +// entries backed by the shared memory cache (that could, in theory, get +// nibbled bytes from that cache, but there is no such memoryIn code). +if (mem_obj-inmem_lo) // in memory cache, but got nibbled at return 0; +// The following check is correct but useless at this position. TODO: Move +// it up when the shared memory cache can either replenish locally nibbled +// bytes or, better, does not use local RAM copy at all. +// if (mem_obj-memCache.index = 0) // backed by a shared memory cache +//return 1; + return 1; } I can adjust or revert this if needed, of course. Thank you, Alex.
Re: [PATCH] Fixes to get more collapsed forwarding hits
On 06/24/2014 01:44 PM, Kinkie wrote: Amos already +1-ed the patch, I have no insights to add so unless someone speaks up real fast, we proceed. Committed to trunk as r13476 and r13477. Thank you, Alex. On Tue, Jun 24, 2014 at 9:29 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/24/2014 12:55 PM, Kinkie wrote: Did you run coadvisor and polygraph against it? Co-Advisor does not have collapsed forwarding cases (there are no explicit RFC 2616 MUSTs that cover CF although some cases can be certainly added to test some MUSTs in a CF context). Polygraph can be used to simulate CF-heavy environment, but all you will get from this patch is an improved hit ratio. It is not going to tell you which collapsing cases Squid have missed. If not, and if the branch is public, it's trivial tor un them against it now (thanks TMF, thanks Pavel!). It doesn't guarantee to catch all cases, but at least it should raise confidence. There is currently no branch for this -- just a trunk patch. A bigger problem is that you will need a different Polygraph workload -- the one we use for Squid regression testing via Jenkins is unlikely to produce a lot of CF opportunities. Overall, while I would certainly welcome Co-Advisor and Polygraph results, I decided to rely on existing trunk regression testing and not spend more time on organizing custom pre-commit tests. If somebody wants to volunteer to test this, please let me know. Thank you, Alex. On Tue, Jun 24, 2014 at 7:37 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/23/2014 11:58 PM, Amos Jeffries wrote: On 24/06/2014 4:07 p.m., Alex Rousskov wrote: * Do not abandon writing a collapsed cache entry when we cannot cache the entry in RAM if the entry can be cached on disk instead. Both shared memory cache and the disk cache have to refuse to cache the entry for it to become non-collapsible. This dual refusal is difficult to detect because each cache may make the caching decision at different times. Added StoreEntry methods to track those decisions and react to them. Hmm. FTR: IMHO the only reasons a response should be non-collapsible is if it is a) private, b) authenticated to another user, c) explicitly known non-cacheable, d) a non-matching variant of the URL, or e) we already discarded some body bytes. Yes, there are many only reasons why a we may not be able to collapse what looked like a collapsible transaction at the beginning. The bullet we are discussing fixes a case where the Squid shared memory caching code said if we cannot cache this entry in the shared memory cache, then we cannot collapse onto this entry. That was wrong; we should still be able to collapse if the disk cache stores the entry. The factor of memory/disk caches not having provided cacheable size allow/reject result yet is no reason to be non-collapsible. It is a reason to wait on that result to determine the cacheability condition (c). Yes, kind of. Today, Squid treats HTTP cachable and storable in a Squid cache as two rather different things. The two conditions are checked and managed in different code places and at different times (with some overlap). And some of those decisions are unrelated to the entry size. However, they are essentially the same from user-perceived caching point of view and can be listed under a single explicitly known non-cacheable bullet if one is careful to interpret that correctly. +1. Although I do not know enough about store yet to do much in-depth audit of the logic changes. FWIW, knowing enough about Store makes me even less confident about changes like these ones. This is very tricky stuff not just because of the complexity of the features involved (caching, collapsing, etc.) but because there are very few simple invariants in Store. I rarely know for sure what I am going to break when changing this stuff. The code is getting better, and I am trying to document the invariants I find, but it is a very slow process, and there will be many more bugs, possibly from this patch as well. Cheers, Alex.
