Re: Multiple issues in Squid-3.2.3 SMP + rock + aufs + a bit of load
tor 2012-11-29 klockan 15:08 +1300 skrev Amos Jeffries: > Does anybody know the rationale for having xmemset at all? I don't see muhc of a meaningful rationale. memory debugging tools already handle it without any wrapper. Regards Henrik
Re: [PATCH] Refactoring url to original url (storeurl step 1)
On 11/28/2012 6:36 PM, Alex Rousskov wrote: On 11/27/2012 11:37 PM, Eliezer Croitoru wrote: The patch is refactoring of two variables: mem_object->url into mem_obj->original_url Store_entry->url into Store_entry->originalUrl Both are named inline with other variables in their scope. This is the first step towards Store_url completion. Please consider documenting the renamed method and data member by adding a brief doxygen comment for them. After spending so much time with this code, you probably know what these members are! :-) I kind of hope I know it :D Now I remember only little about it and I know more about specific other points in the code that make use of it. But I will try to put it in my list. The new comment in StoreMetaURL::checkConsistency() does not belong to this patch, IMO. If you insist on keeping it, then please spellcheck "rewritted". OK, I will add it later on. Overall, I think this change is a very important step, but it should not be committed until we have other steps (or at all): * The primary point of this change is to track cases where the rewritten "store URL" should be used instead of the "original URL". If we commit this patch now, we will not longer be able to track those cases when reviewing your future patches. It seems to me This is not the real idea. A while back We discussed about it and Henrik if i'm not wrong suggested what you did. The basic idea of this major change is to take the "url" statement and change the meaning of it to something usable. I do not expect anyone that works with squid code ever to understand the exact reasons I did the change since they almost always dont know other code. But These point of change do reflect in major points of the code the distinction between the "store url" which is the original url and to the "store rewritten url". * There is no need to create hundreds of conflicts with pending and future changes for the renaming sake alone. At the very end of the project, the renaming-only changes can be removed OR, if we decide that they should stay, there will be a compelling reason to keep them -- we would get the store_url feature as a payoff. Indeed if it was avoidable I would agree with you. At the beginning I thought the same but since I researched squid code for a long time to make it work it seems unlikely to understand the code without changing it(unless you know the code like the palm of your hand). Consider For now that the only distance between the feature to be from beta to stable is that the code wouldn't change every couple days or Basic changes would be done to allow slowly migrating the code changes. Just notice that this is the most painful patch in the whole feature. Thank you, Alex. Eliezer -- Eliezer Croitoru https://www1.ngtech.co.il sip:ngt...@sip2sip.info IT consulting for Nonprofit organizations eliezer ngtech.co.il
Re: ICAP vectoring points
Hey Steve, I was just wondering what exactly you need to do? What is the goal\task of the ICAP server. There are pretty good ways to do it with the current ICAP implementation. I found my self doing things I couldn't have imaging that I can do with ICAP. The only missing thing is that you cant pass yet to the ICAP server special custom headers by your choose. If you have specific ICAP solution maybe it's not design to even do what you need. Regards, Eliezer On 11/28/2012 8:26 PM, Steve Hill wrote: On 28.11.12 16:19, Alex Rousskov wrote: Yes, it is both very far from trivial and, in most cases, avoidable by redesigning the adaptation approach itself. I have seen many use cases that started with a post-cache requirement, but were successfully massaged into avoiding one. Can you explain this in a little more detail? The results of my ICAP server are dependent on the user that is requesting the object, so doing the work in respmod_precache would result in the modified object going into the cache and then being retrieved by other users who would need different modifications. I can't really see a way around this other than making the objects all uncachable, which rather defeats the purpose of having a cache. :) At the moment I'm working around this problem by having 2 Squids - the upstream one is running the cache and the downstream one is talking the the ICAP server and has the cache disabled. It would be nice to be able to remove this kludge, especially since it is causing a few issues at the moment. Please make sure the new code is agnostic to the ICAP/eCAP difference, just like the existing adaptation code. If you find yourself accessing ICAP-specific code for post-cache adaptation needs, you are probably doing something wrong. Thank you for the pointers - it may be many months until I get chance to properly look into it (if at all), but at least I have a few ideas where to begin when I have a bit of spare time. -- Eliezer Croitoru https://www1.ngtech.co.il sip:ngt...@sip2sip.info IT consulting for Nonprofit organizations eliezer ngtech.co.il
Re: Multiple issues in Squid-3.2.3 SMP + rock + aufs + a bit of load
On 29.11.2012 12:07, Henrik Nordström wrote:
While trying to set up an advanced Squid-3.2.3 with 4 workers, a
number
of rock stores for different objects and per-worker aufs caches for
larger files I ran into multiple issues.
- cache.log completely flooded with memset() messages at startup with
full debug enabled.
--- squid-3.2.3/src/ipc/StoreMap.cc 2012-10-20 15:39:49.0
+0300
+++ squid-3.2.3/src/ipc/StoreMap.cc 2012-11-20 20:55:41.089500435
+0200
@@ -272,8 +272,8 @@
Ipc::StoreMapSlot::StoreMapSlot(): state(Empty)
{
-xmemset(&key, 0, sizeof(key));
-xmemset(&basics, 0, sizeof(basics));
+memset(&key, 0, sizeof(key));
+memset(&basics, 0, sizeof(basics));
}
Does anybody know the rationale for having xmemset at all?
Should we drop it completely from the code and reduce the dependencies
for some code a bit?
or should we shuffle it into the leakfinder or profiler code as a
#define?
Either way IMO it looks like we should drop the 'x' version from the
rest of the code base by default.
Amos
Multiple issues in Squid-3.2.3 SMP + rock + aufs + a bit of load
While trying to set up an advanced Squid-3.2.3 with 4 workers, a number
of rock stores for different objects and per-worker aufs caches for
larger files I ran into multiple issues.
- Quite frequent NULL pointer assertions failures in peer selection if
the client aborts before peer selection have completed. Guess this is
seen whenever there is peering enabled and sufficiently impatient
clients around.
- each worker restarting at startup due to rock store DB open failures
(timeout). This seems to settle once the rock store have completed
rebuilding, but not 100% sure that's the cause as logs are a bit
inclusive. Might also be related to the delay due to the kid rebuilding
it's own aufs cache_dir.
- kid registration failures at startup (timeout). This seem to be due to
the local cache rebuild in each kid taking more than 6 seconds to
complete, confusing the registration timeout management.
- cache.log completely flooded with memset() messages at startup with
full debug enabled.
The attached patch tries to address or work around the above issues. But
have not fully understood the timeouts. Suspect the timeours are false
and caused by each kid doing a store rebuild at startup which takes a
bit of time so those changes are likely only a workaround and not
actually addressing the root problem.
Will also set up bug reports for these, but sending here first so it
doesn't get lost by mistake.
Regards
Henrik
--- squid-3.2.3/src/DiskIO/IpcIo/IpcIoFile.cc 2012-10-20 15:39:49.0
+0300
+++ squid-3.2.3/src/DiskIO/IpcIo/IpcIoFile.cc 2012-11-29 00:47:11.451848433
+0200
@@ -30,7 +30,7 @@
// TODO: make configurable or compute from squid.conf settings if possible
static const int QueueCapacity = 1024;
-const double IpcIoFile::Timeout = 7; // seconds; XXX: ALL,9 may require more
+const double IpcIoFile::Timeout = 300; // seconds; XXX: ALL,9 may require more
IpcIoFile::IpcIoFileList IpcIoFile::WaitingForOpen;
IpcIoFile::IpcIoFilesMap IpcIoFile::IpcIoFiles;
std::auto_ptr IpcIoFile::queue;
@@ -218,7 +218,7 @@
{
bool ioError = false;
if (!response) {
-debugs(79, 3, HERE << "error: timeout");
+debugs(79, 1, HERE << "error: timeout");
ioError = true; // I/O timeout does not warrant setting error_?
} else {
if (response->xerrno) {
--- squid-3.2.3/src/HttpHeader.cc 2012-10-20 15:39:49.0 +0300
+++ squid-3.2.3/src/HttpHeader.cc 2012-11-21 19:51:23.826807754 +0200
@@ -548,7 +548,7 @@
char *nulpos;
if ((nulpos = (char*)memchr(header_start, '\0', header_end -
header_start))) {
-debugs(55, 1, "WARNING: HTTP header contains NULL characters {" <<
+debugs(55, 2, "WARNING: HTTP header contains NULL characters {" <<
getStringPrefix(header_start, nulpos) << "}\nNULL\n{" <<
getStringPrefix(nulpos+1, header_end));
goto reset;
}
--- squid-3.2.3/src/ipc/StoreMap.cc 2012-10-20 15:39:49.0 +0300
+++ squid-3.2.3/src/ipc/StoreMap.cc 2012-11-20 20:55:41.089500435 +0200
@@ -272,8 +272,8 @@
Ipc::StoreMapSlot::StoreMapSlot(): state(Empty)
{
-xmemset(&key, 0, sizeof(key));
-xmemset(&basics, 0, sizeof(basics));
+memset(&key, 0, sizeof(key));
+memset(&basics, 0, sizeof(basics));
}
void
--- squid-3.2.3/src/ipc/Strand.cc 2012-10-20 15:39:49.0 +0300
+++ squid-3.2.3/src/ipc/Strand.cc 2012-11-29 00:12:12.385439783 +0200
@@ -53,7 +53,7 @@
TypedMsgHdr message;
ann.pack(message);
SendMessage(coordinatorAddr, message);
-setTimeout(6, "Ipc::Strand::timeoutHandler"); // TODO: make 6 configurable?
+setTimeout(600, "Ipc::Strand::timeoutHandler"); // TODO: make 6
configurable?
}
void Ipc::Strand::receive(const TypedMsgHdr &message)
--- squid-3.2.3/src/peer_select.cc 2012-10-20 15:39:49.0 +0300
+++ squid-3.2.3/src/peer_select.cc 2012-11-21 12:01:06.030883260 +0200
@@ -241,7 +241,7 @@
(req->flags.intercepted ||
req->flags.spoof_client_ip);
const bool useOriginalDst = Config.onoff.client_dst_passthru ||
!req->flags.hostVerified;
const bool choseDirect = fs && fs->code == HIER_DIRECT;
-if (isIntercepted && useOriginalDst && choseDirect) {
+if (isIntercepted && useOriginalDst && choseDirect &&
req->clientConnectionManager.valid()) {
// construct a "result" adding the ORIGINAL_DST to the set instead of
DIRECT
Comm::ConnectionPointer p = new Comm::Connection();
p->remote = req->clientConnectionManager->clientConnection->local;
--- squid-3.2.3/src/DiskIO/IpcIo/IpcIoFile.cc 2012-10-20 15:39:49.0 +0300
+++ squid-3.2.3/src/DiskIO/IpcIo/IpcIoFile.cc 2012-11-29 00:47:11.451848433 +0200
@@ -30,7 +30,7 @@
// TODO: make configurable or compute from squid.conf settings if possible
static const int QueueCapacity = 1024;
-const double IpcIoFile::Timeout = 7; // seconds; XXX: ALL,9 may require more
+const double IpcIoFile::Timeout = 300; //
Re: ICAP vectoring points
On 28.11.12 16:19, Alex Rousskov wrote: Yes, it is both very far from trivial and, in most cases, avoidable by redesigning the adaptation approach itself. I have seen many use cases that started with a post-cache requirement, but were successfully massaged into avoiding one. Can you explain this in a little more detail? The results of my ICAP server are dependent on the user that is requesting the object, so doing the work in respmod_precache would result in the modified object going into the cache and then being retrieved by other users who would need different modifications. I can't really see a way around this other than making the objects all uncachable, which rather defeats the purpose of having a cache. :) At the moment I'm working around this problem by having 2 Squids - the upstream one is running the cache and the downstream one is talking the the ICAP server and has the cache disabled. It would be nice to be able to remove this kludge, especially since it is causing a few issues at the moment. Please make sure the new code is agnostic to the ICAP/eCAP difference, just like the existing adaptation code. If you find yourself accessing ICAP-specific code for post-cache adaptation needs, you are probably doing something wrong. Thank you for the pointers - it may be many months until I get chance to properly look into it (if at all), but at least I have a few ideas where to begin when I have a bit of spare time. -- - Steve Hill Technical Director Opendium Limited http://www.opendium.com Direct contacts: Instant messager: xmpp:st...@opendium.com Email:st...@opendium.com Phone:sip:st...@opendium.com Sales / enquiries contacts: Email:sa...@opendium.com Phone:+44-844-9791439 / sip:sa...@opendium.com Support contacts: Email:supp...@opendium.com Phone:+44-844-4844916 / sip:supp...@opendium.com
Re: [PATCH] Refactoring url to original url (storeurl step 1)
On 11/27/2012 11:37 PM, Eliezer Croitoru wrote: > The patch is refactoring of two variables: > mem_object->url into mem_obj->original_url > Store_entry->url into Store_entry->originalUrl > > Both are named inline with other variables in their scope. > This is the first step towards Store_url completion. Please consider documenting the renamed method and data member by adding a brief doxygen comment for them. After spending so much time with this code, you probably know what these members are! :-) The new comment in StoreMetaURL::checkConsistency() does not belong to this patch, IMO. If you insist on keeping it, then please spellcheck "rewritted". Overall, I think this change is a very important step, but it should not be committed until we have other steps (or at all): * The primary point of this change is to track cases where the rewritten "store URL" should be used instead of the "original URL". If we commit this patch now, we will not longer be able to track those cases when reviewing your future patches. * There is no need to create hundreds of conflicts with pending and future changes for the renaming sake alone. At the very end of the project, the renaming-only changes can be removed OR, if we decide that they should stay, there will be a compelling reason to keep them -- we would get the store_url feature as a payoff. Thank you, Alex.
Re: ICAP vectoring points
On 11/28/2012 04:16 AM, Steve Hill wrote: > I have a requirement for a respmod_postcache vectoring point (ICAP > server is between the cache and the client and intercepts responses). > I've not done a lot of work on the Squid internals, so would anyone be > able to give me some pointers on what is involved in implementing this > and where to start looking? Am I correct in thinking that since this > hasn't been implemented yet, it is reasonably non-trivial? Hi Steve, Yes, it is both very far from trivial and, in most cases, avoidable by redesigning the adaptation approach itself. I have seen many use cases that started with a post-cache requirement, but were successfully massaged into avoiding one. FWIW, none of the competing proxies support post-cache ICAP adaptations AFAIK. IMO, post-cache adaptations support in Squid should be welcomed, but properly adding it to the current client-side code mess would be difficult. A better plan may be to restructure and streamline the client-side code first. BTW, adaptation needs drove a lot of server-side code polishing as well, but it took many [non-consecutive] months, and the server-side code was arguably in a better shape to start with. The existing adaptation code should work well with all four standard vectoring points. It is the vectoring point code itself that presents a challenge here. If you are looking for starting pointers, search for Adaptation namespace use in Server.cc and client_side*.cc. There are not so many of them, but do not be deceived by the apparent simplicity of that code: The difficulty is hidden in the asynchronous interaction between adaptation buffers/streams/decisions and client/server side buffers/streams/decisions. On the client side, you will probably need to inject post-cache adaptation when the store_client is receiving a response (see StoreClient.h). Please make sure the new code is agnostic to the ICAP/eCAP difference, just like the existing adaptation code. If you find yourself accessing ICAP-specific code for post-cache adaptation needs, you are probably doing something wrong. HTH, Alex.
ICAP vectoring points
Squid currently implements ICAP vectoring points for reqmod_precache (ICAP server is between the client and the cache and intercepting requests) and respmod_precache (ICAP server is between the server and the cache and intercepting responses). I have a requirement for a respmod_postcache vectoring point (ICAP server is between the cache and the client and intercepts responses). I've not done a lot of work on the Squid internals, so would anyone be able to give me some pointers on what is involved in implementing this and where to start looking? Am I correct in thinking that since this hasn't been implemented yet, it is reasonably non-trivial? Thanks. -- - Steve Hill Technical Director Opendium Limited http://www.opendium.com Direct contacts: Instant messager: xmpp:st...@opendium.com Email:st...@opendium.com Phone:sip:st...@opendium.com Sales / enquiries contacts: Email:sa...@opendium.com Phone:+44-844-9791439 / sip:sa...@opendium.com Support contacts: Email:supp...@opendium.com Phone:+44-844-4844916 / sip:supp...@opendium.com
