You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Davi Arnaut <da...@haxent.com.br> on 2006/10/27 16:38:02 UTC
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
minfrin@apache.org wrote:
> Author: minfrin
> Date: Fri Oct 27 06:28:56 2006
> New Revision: 468373
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=468373
> Log:
> mod_cache: Pass the output filter stack through the store_body()
> hook, giving each cache backend the ability to make a better
> decision as to how it will allocate the tasks of writing to the
> cache and writing to the network. Previously the write to the
> cache task needed to be complete before the same brigade was
> written to the network, and this caused timing and memory issues
> on large cached files. This fix replaces the previous fix for
> PR39380.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/cache/mod_cache.c
> httpd/httpd/trunk/modules/cache/mod_cache.h
> httpd/httpd/trunk/modules/cache/mod_disk_cache.c
> httpd/httpd/trunk/modules/cache/mod_disk_cache.h
> httpd/httpd/trunk/modules/cache/mod_mem_cache.c
>
..
> @@ -1286,9 +1292,15 @@
> static apr_status_t open_new_file(request_rec *r, const char *filename,
> apr_file_t **fd, disk_cache_conf *conf)
> {
> - int flags = APR_CREATE | APR_WRITE | APR_BINARY | APR_BUFFERED | APR_EXCL;
> + int flags;
> apr_status_t rv;
>
> + flags = APR_CREATE | APR_WRITE | APR_READ | APR_BINARY | APR_BUFFERED | APR_EXCL | APR_TRUNCATE;
Nitpick: APR_TRUNCATE here is useless.
> +#if APR_HAS_SENDFILE
> + flags |= ((pdconf->enable_sendfile == ENABLE_SENDFILE_OFF)
> + ? 0 : APR_SENDFILE_ENABLED);
> +#endif
> +
Where is pdconf ? Check out all those APR_HAS_SENDFILE.
> while(1) {
> rv = apr_file_open(fd, filename, flags,
> APR_FPROT_UREAD | APR_FPROT_UWRITE, r->pool);
> @@ -1611,150 +1623,50 @@
> return APR_SUCCESS;
> }
>
Looking at open_new_file, it is somewhat unreliable:
if(finfo.mtime < (apr_time_now() - conf->updtimeout) ) {
We use APR_BUFFERED and under various circumstances files may get
modified without having the mtime updated.
..
> +/**
> + * Store the body of the response in the disk cache.
> + *
> + * As the data is written to the cache, it is also written to
> + * the filter provided. On network write failure, the full body
> + * will still be cached.
> + */
> +static apr_status_t store_body(cache_handle_t *h, ap_filter_t *f, apr_bucket_brigade *bb)
> {
> - apr_bucket *e;
> + apr_bucket *e, *b;
> + request_rec *r = f->r;
> apr_status_t rv;
> - int copy_file = FALSE;
> disk_cache_object_t *dobj = (disk_cache_object_t *) h->cache_obj->vobj;
> disk_cache_conf *conf = ap_get_module_config(r->server->module_config,
> &disk_cache_module);
>
> dobj->store_body_called++;
> -
> +
White space trash.
> if(r->no_cache) {
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
> "disk_cache: store_body called for URL %s even though"
> "no_cache is set", dobj->name);
> file_cache_errorcleanup(dobj, r);
> - return APR_EGENERAL;
> + ap_remove_output_filter(f);
> + return ap_pass_brigade(f->next, bb);
> }
>
> if(dobj->initial_size == 0) {
> /* Don't waste a body cachefile on a 0 length body */
> - return APR_SUCCESS;
> + return ap_pass_brigade(f->next, bb);
> }
>
> if(!dobj->skipstore && dobj->fd == NULL) {
> rv = open_new_file(r, dobj->datafile, &(dobj->fd), conf);
> - if(rv == CACHE_EEXIST) {
> + if (rv == CACHE_EEXIST) {
> /* Someone else beat us to storing this */
> dobj->skipstore = TRUE;
> }
> - else if(rv != APR_SUCCESS) {
> - return rv;
> + else if (rv != APR_SUCCESS) {
> + ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
> + "disk_cache: store_body tried to open cached file "
> + "for URL %s and this failed", dobj->name);
> + ap_remove_output_filter(f);
> + return ap_pass_brigade(f->next, bb);
> }
> else {
> dobj->file_size = 0;
> @@ -1762,194 +1674,227 @@
> }
>
> if(dobj->skipstore) {
> - /* Someone else beat us to storing this object */
> - if(dobj->store_body_called == 1 &&
> - dobj->initial_size > 0 &&
> - APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb)) )
> - {
> - /* Yay, we can replace the body with the cached instance */
> - return replace_brigade_with_cache(h, r, bb);
> - }
> -
> - return APR_SUCCESS;
> + /* Someone else beat us to storing this object.
> + * We are too late to take advantage of this storage :( */
> + ap_remove_output_filter(f);
> + return ap_pass_brigade(f->next, bb);
> }
Those "if(" and "if (" are going on my nerves! :)
..
> + /* start caching the brigade */
> + ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
> + "disk_cache: Caching body for URL %s", dobj->name);
>
> - }
> - else {
> - ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
> - "disk_cache: Caching body for URL %s", dobj->name);
> + e = APR_BRIGADE_FIRST(bb);
> + while (e != APR_BRIGADE_SENTINEL(bb)) {
>
> - for (e = APR_BRIGADE_FIRST(bb);
> - e != APR_BRIGADE_SENTINEL(bb);
> - e = APR_BUCKET_NEXT(e))
> - {
> - const char *str;
> - apr_size_t length, written;
> + const char *str;
> + apr_size_t length, written;
> + apr_off_t offset = 0;
>
> - /* Ignore the non-data-buckets */
> - if(APR_BUCKET_IS_METADATA(e)) {
> - continue;
> - }
> + /* try write all data buckets to the cache, except for metadata buckets */
> + if(!APR_BUCKET_IS_METADATA(e)) {
Swapping the metadata check would make the code much more readable:
if (is_metadata)
deal with it
continue
handle normal bucket
> + /* read in a bucket fragment */
> rv = apr_bucket_read(e, &str, &length, APR_BLOCK_READ);
> if (rv != APR_SUCCESS) {
> ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
> - "disk_cache: Error when reading bucket for URL %s",
> + "disk_cache: Error when reading bucket for URL %s, aborting request",
> dobj->name);
> file_cache_errorcleanup(dobj, r);
> + /* not being able to read the bucket is fatal,
> + * return this up the filter stack
> + */
> return rv;
> }
> +
> + /* try write the bucket fragment to the cache */
> + apr_file_seek(dobj->fd, APR_END, &offset);
> rv = apr_file_write_full(dobj->fd, str, length, &written);
> - if (rv != APR_SUCCESS) {
> + offset = - (apr_off_t)written;
> + apr_file_seek(dobj->fd, APR_END, &offset);
> + /* if the cache write was successful, swap the original bucket
> + * with a file bucket pointing to the same data in the cache.
> + *
> + * This is done because:
> + *
> + * - The ap_core_output_filter can take advantage of its ability
> + * to do non blocking writes on file buckets.
> + *
> + * - We are prevented from the need to read the original bucket
> + * a second time inside ap_core_output_filter, which could be
> + * expensive or memory consuming.
> + *
> + * - The cache, in theory, should be faster than the backend,
> + * otherwise there would be little point in caching in the first
> + * place.
> + */
> + if (APR_SUCCESS == rv) {
> +
> + /* remove and destroy the original bucket from the brigade */
> + b = e;
> + e = APR_BUCKET_NEXT(e);
> + APR_BUCKET_REMOVE(b);
> + apr_bucket_destroy(b);
aka as apr_bucket_delete
> + /* Is our network connection still alive?
> + * If not, we must continue caching the file, so keep looping.
> + * We will return the error at the end when caching is done.
> + */
> + if (APR_SUCCESS == dobj->frv) {
> +
> + /* insert a file bucket pointing to the cache into out temporary brigade */
> + if (diskcache_brigade_insert(dobj->tmpbb, dobj->fd, dobj->file_size,
> + written,
> + dobj->updtimeout, r->pool) == NULL) {
> + return APR_ENOMEM;
> + }
Err. We had the data in memory, we are going to read it back from disk
again just in order to not block ? That's nonsense.
We don't need a special bucket type!
Also, are you really sure you are tracking correctly all those buckets
with the correct offset ? The file pointer offset is unique for a single fd.
I will stop the review here, I have to calm down to really digest what's
happening.
--
Davi Arnaut
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
Davi Arnaut wrote:
> But that's a corner case. There is no reason in doing this for small
> files (common case). For example, in a enterprise grade server memory is
> cheap and permanent storage is slow and expensive.
If permanent storage is slow and expensive, what on earth is the admin
doing trying to turn on a disk cache?
However luckily though the typical modern OS helps by RAM caching common
disk access, the argument has been made here before that a disk cache
gives same or better performance than the memory cache.
Please don't forget that mod_cache, by design, is a generic RFC2616
caching proxy bolted onto the frontend of the webserver, and that means
you can cache virtually any well behaved application - from mod_perl, to
CGI, to mod_python, mod_proxy, whatever.
And also don't forget that compared to the speed at which a webserver
can generate data, the typical client is many orders of maginitude
slower at swallowing that data. So your big expensive mod_perl, CGI,
mod_python, etc sits around waiting for the client to get on with it and
download the response, using resources better spent elsewhere.
A very common use case is for a caching proxy is to take the load off an
expensive application server, and usually this is deployed as a separate
piece of kit in front of the webserver. With mod_cache, you can offload
this inside the server itself, which simplifies your setup.
In short, this is not a corner case.
>> Also, httpd seems to be faster delivering data by sendfile than
>> delivering data from memory buckets. That's more of a performance bug
>> in httpd though.
>
> It's not all httpd fault. Sendfile avoids memory copying/context
> switches/etc behind the scenes (in the kernel) because it "talks"
> directly to the underlying file system. When you have the memory already
> in user space there isn't much that you can do, the kernel has to copy
> it..etc.
Exactly - another strong argument to make sure that when you turn on the
disk cache, you serve the network off the disk cache.
Regards,
Graham
--
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
Davi Arnaut wrote:
> Because the data is already in memory. Why should I write something to
> disk, erase it from memory, and read it again shortly ? Why should I
> take care of something that is the job of the OS ? Why should I trash
> the VM constantly ?
Because reading slowly from a cache disk is less resource intensive than
reading from CGI, proxy, slow NFS drives, etc. And reading from cache
disk gives you SENDFILE. Try it out, you will see.
Regards,
Graham
--
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Davi Arnaut <da...@haxent.com.br>.
Davi Arnaut wrote:
> Greg Marr wrote:
>> At 01:41 PM 10/27/2006, Davi Arnaut wrote:
>>> Niklas Edmundsson wrote:
>>>> And when you have a file backend, you want to hit your disk cache
>>> and
>>>> not the backend when delivering data to a client. People might
>>> think
>>>> that this doesn't matter, but for large files, especially larger
>>> than
>>>> RAM in your machine, you usually go disk-bound without much help
>>> from
>>>> the OS disk cache.
>>> But that's a corner case. There is no reason in doing this for small
>>> files (common case). For example, in a enterprise grade server
>>> memory is
>>> cheap and permanent storage is slow and expensive.
>> So why would this server be using mod_disk_cache in that
>> case? Shouldn't this server be using mod_mem_cache? Selecting
>> mod_disk_cache over mod_mem_cache means it's better to serve the
>> cache from disk rather than from memory. If serving from the disk on
>> the original request is too slow, then wouldn't serving from the disk
>> on the subsequent requests be too slow as well?
>>
>
> Because the data is already in memory. Why should I write something to
> disk, erase it from memory, and read it again shortly ? Why should I
> take care of something that is the job of the OS ? Why should I trash
> the VM constantly ?
I should note that if there is enough memory those reads will be served
by the page cache. Later we could think about reading directly to
mmapable files, changing it's protection (from rw to ro) as soon as we
are done writing to.
--
Davi Arnaut
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Davi Arnaut <da...@haxent.com.br>.
Greg Marr wrote:
> At 01:41 PM 10/27/2006, Davi Arnaut wrote:
>> Niklas Edmundsson wrote:
>>> And when you have a file backend, you want to hit your disk cache
>> and
>>> not the backend when delivering data to a client. People might
>> think
>>> that this doesn't matter, but for large files, especially larger
>> than
>>> RAM in your machine, you usually go disk-bound without much help
>> from
>>> the OS disk cache.
>> But that's a corner case. There is no reason in doing this for small
>> files (common case). For example, in a enterprise grade server
>> memory is
>> cheap and permanent storage is slow and expensive.
>
> So why would this server be using mod_disk_cache in that
> case? Shouldn't this server be using mod_mem_cache? Selecting
> mod_disk_cache over mod_mem_cache means it's better to serve the
> cache from disk rather than from memory. If serving from the disk on
> the original request is too slow, then wouldn't serving from the disk
> on the subsequent requests be too slow as well?
>
Because the data is already in memory. Why should I write something to
disk, erase it from memory, and read it again shortly ? Why should I
take care of something that is the job of the OS ? Why should I trash
the VM constantly ?
A proxy/cache common purpose is to cache web content, this means we have
a large number of small files (think html pages, images, etc) that we
must keep in a permanent storage, but if we have memory, let's use it!
--
Davi Arnaut
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES
modules/cache/mod_cache.c modules/cache/mod_cache.h
modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Greg Marr <gr...@alum.wpi.edu>.
At 01:41 PM 10/27/2006, Davi Arnaut wrote:
>Niklas Edmundsson wrote:
> > And when you have a file backend, you want to hit your disk cache
> and
> > not the backend when delivering data to a client. People might
> think
> > that this doesn't matter, but for large files, especially larger
> than
> > RAM in your machine, you usually go disk-bound without much help
> from
> > the OS disk cache.
>
>But that's a corner case. There is no reason in doing this for small
>files (common case). For example, in a enterprise grade server
>memory is
>cheap and permanent storage is slow and expensive.
So why would this server be using mod_disk_cache in that
case? Shouldn't this server be using mod_mem_cache? Selecting
mod_disk_cache over mod_mem_cache means it's better to serve the
cache from disk rather than from memory. If serving from the disk on
the original request is too slow, then wouldn't serving from the disk
on the subsequent requests be too slow as well?
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Davi Arnaut <da...@haxent.com.br>.
Niklas Edmundsson wrote:
> On Fri, 27 Oct 2006, Graham Leggett wrote:
>
>>>> Err. We had the data in memory, we are going to read it back from disk
>>>> again just in order to not block ? That's nonsense.
>>> Agreed.
>> Please explain.
>>
>> This is a disk cache. Why would you write expensive bucket data to cache,
>> and then expensive bucket data to the network?
>>
>> That's plain stupid.
>
> And when you have a file backend, you want to hit your disk cache and
> not the backend when delivering data to a client. People might think
> that this doesn't matter, but for large files, especially larger than
> RAM in your machine, you usually go disk-bound without much help from
> the OS disk cache.
But that's a corner case. There is no reason in doing this for small
files (common case). For example, in a enterprise grade server memory is
cheap and permanent storage is slow and expensive.
> Also, httpd seems to be faster delivering data by sendfile than
> delivering data from memory buckets. That's more of a performance bug
> in httpd though.
It's not all httpd fault. Sendfile avoids memory copying/context
switches/etc behind the scenes (in the kernel) because it "talks"
directly to the underlying file system. When you have the memory already
in user space there isn't much that you can do, the kernel has to copy
it..etc.
--
Davi Arnaut
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES
modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c
modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c
Posted by Niklas Edmundsson <ni...@acc.umu.se>.
On Fri, 27 Oct 2006, Graham Leggett wrote:
>>> Err. We had the data in memory, we are going to read it back from disk
>>> again just in order to not block ? That's nonsense.
>>
>> Agreed.
>
> Please explain.
>
> This is a disk cache. Why would you write expensive bucket data to cache,
> and then expensive bucket data to the network?
>
> That's plain stupid.
And when you have a file backend, you want to hit your disk cache and
not the backend when delivering data to a client. People might think
that this doesn't matter, but for large files, especially larger than
RAM in your machine, you usually go disk-bound without much help from
the OS disk cache.
Also, httpd seems to be faster delivering data by sendfile than
delivering data from memory buckets. That's more of a performance bug
in httpd though.
/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se | nikke@acc.umu.se
---------------------------------------------------------------------------
Monolith Auto Sales Center: "My God! It's full of cars!"
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES
modules/cache/mod_cache.c modules/cache/mod_cache.h
modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
On Fri, October 27, 2006 6:00 pm, Joe Orton wrote:
>> Err. We had the data in memory, we are going to read it back from disk
>> again just in order to not block ? That's nonsense.
>
> Agreed.
Please explain.
This is a disk cache. Why would you write expensive bucket data to cache,
and then expensive bucket data to the network?
That's plain stupid.
>> We don't need a special bucket type!
>
> Agreed. So -1 on this, on r462696, and on r450105.
Sigh. So not only have you not reviewed the code, you have not reviewed
the comment in CHANGES either. r450105 was reversed by this patch.
I'm afraid we do need a special bucket type.
The current file bucket type does not currently have the ability to
support the same fd across multiple buckets. Sure you can split one bucket
to make two, but you cannot make two file buckets out of one fd without
some low level hocus pocus.
Regards,
Graham
--
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Oct 27, 2006 at 11:38:02AM -0300, Davi Arnaut wrote:
> > + /* Is our network connection still alive?
> > + * If not, we must continue caching the file, so keep looping.
> > + * We will return the error at the end when caching is done.
> > + */
> > + if (APR_SUCCESS == dobj->frv) {
> > +
> > + /* insert a file bucket pointing to the cache into out temporary brigade */
> > + if (diskcache_brigade_insert(dobj->tmpbb, dobj->fd, dobj->file_size,
> > + written,
> > + dobj->updtimeout, r->pool) == NULL) {
> > + return APR_ENOMEM;
> > + }
>
> Err. We had the data in memory, we are going to read it back from disk
> again just in order to not block ? That's nonsense.
Agreed.
> We don't need a special bucket type!
Agreed. So -1 on this, on r462696, and on r450105.
joe
Re: [PATCH] mod_disk_cache fails to compile [Was: svn commit:
r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
modules/cache/mod_cache.h modules/cache/mod_disk_cache.c
modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c]
Posted by Graham Leggett <mi...@sharp.fm>.
On Thu, November 2, 2006 12:34 pm, Martin Kraemer wrote:
> Still, since 2 days, again mod_disk_cache fails to compile on
> platforms with sendfile (e.g., FreeBSD).
I missed the patch you sent - looks the same as r468409 though?
Regards,
Graham
--
Re: [PATCH] mod_disk_cache fails to compile [Was: svn commit:
r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
modules/cache/mod_cache.h modules/cache/mod_disk_cache.c
modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c]
Posted by Graham Leggett <mi...@sharp.fm>.
On Thu, November 2, 2006 2:20 pm, Martin Kraemer wrote:
> Oops - I forgot to mention the branch I'm working on - it's a bug in
> the trunk. I am at Revision 470348, but the pdconf variable occurs at
> *two* places in the file, however the declaration appears only once.
Aaah... the penny drops.
I deleted the pdconf definition in one place (it was giving a compiler
warning for the APR_HAS_SENDFILE == 0 case), and when it was pointed out I
put it back.
Looks like this code hasn't built in the SENDFILE case for a while.
Regards,
Graham
--
Re: [PATCH] mod_disk_cache fails to compile [Was: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c]
Posted by Martin Kraemer <ma...@apache.org>.
On Thu, Nov 02, 2006 at 01:00:29PM +0200, Graham Leggett wrote:
> On Thu, November 2, 2006 12:34 pm, Martin Kraemer wrote:
>
> > Still, since 2 days, again mod_disk_cache fails to compile on
> > platforms with sendfile (e.g., FreeBSD).
>
Oops - I forgot to mention the branch I'm working on - it's a bug in
the trunk. I am at Revision 470348, but the pdconf variable occurs at
*two* places in the file, however the declaration appears only once.
Martin
--
<Ma...@Fujitsu-Siemens.com> | Fujitsu Siemens
Fon: +49-89-636-46021, FAX: +49-89-636-48332 | 81730 Munich, Germany
Re: [PATCH] mod_disk_cache fails to compile [Was: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c]
Posted by Martin Kraemer <ma...@apache.org>.
On Thu, Nov 02, 2006 at 01:00:29PM +0200, Graham Leggett wrote:
> If you can send more details, I can get to the bottom of it -
> "fails to compile" doesn't tell me anything useful.
mod_disk_cache.c: In function `open_new_file':
mod_disk_cache.c:1304: `pdconf' undeclared (first use in this function)
mod_disk_cache.c:1304: (Each undeclared identifier is reported only once
mod_disk_cache.c:1304: for each function it appears in.)
--
<Ma...@Fujitsu-Siemens.com> | Fujitsu Siemens
Fon: +49-89-636-46021, FAX: +49-89-636-48332 | 81730 Munich, Germany
Re: [PATCH] mod_disk_cache fails to compile [Was: svn commit:
r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
modules/cache/mod_cache.h modules/cache/mod_disk_cache.c
modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c]
Posted by Graham Leggett <mi...@sharp.fm>.
On Thu, November 2, 2006 12:34 pm, Martin Kraemer wrote:
> Still, since 2 days, again mod_disk_cache fails to compile on
> platforms with sendfile (e.g., FreeBSD).
Is r468409 present in your tree?
Unfortunately I don't have a FreeBSD box, and so cannot reproduce the
problem. If you can send more details, I can get to the bottom of it -
"fails to compile" doesn't tell me anything useful.
Regards,
Graham
--
Index: modules/cache/mod_disk_cache.c
===================================================================
--- modules/cache/mod_disk_cache.c (revision 468408)
+++ modules/cache/mod_disk_cache.c (revision 468409)
@@ -720,6 +720,10 @@
apr_time_t starttime = apr_time_now();
int flags;
apr_status_t rc;
+#if APR_HAS_SENDFILE
+ core_dir_config *pdconf = ap_get_module_config(r->per_dir_config,
+ &core_module);
+#endif
flags = APR_READ|APR_BINARY|APR_BUFFERED;
#if APR_HAS_SENDFILE
[PATCH] mod_disk_cache fails to compile [Was: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c]
Posted by Martin Kraemer <ma...@apache.org>.
On Fri, Oct 27, 2006 at 05:05:29PM +0200, Graham Leggett wrote:
> On Fri, October 27, 2006 4:38 pm, Davi Arnaut wrote:
>
> > Where is pdconf ? Check out all those APR_HAS_SENDFILE.
>
> Aaargh... will fix.
Still, since 2 days, again mod_disk_cache fails to compile on
platforms with sendfile (e.g., FreeBSD).
Martin
--
<Ma...@Fujitsu-Siemens.com> | Fujitsu Siemens
Fon: +49-89-636-46021, FAX: +49-89-636-48332 | 81730 Munich, Germany
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES
modules/cache/mod_cache.c modules/cache/mod_cache.h
modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
On Fri, October 27, 2006 6:14 pm, Paul Querna wrote:
> Honestly, I am -1 on any module needing knowledge of the core module
> setting. We should be making the abstraction better, not pulling things
> from the core config.
This is true, but right now we don't have this level of abstraction, and
I'm trying to get the disk cache put to bed first. Baby steps :)
Regards,
Graham
--
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES
modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c
modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c
Posted by Paul Querna <ch...@force-elite.com>.
Graham Leggett wrote:
> On Fri, October 27, 2006 6:05 pm, Niklas Edmundsson wrote:
>
>> The purpose of that code was originally to make "EnableSendfile Off"
>> in the config file work. APR_HAS_SENDFILE only tells you that APR has
>> sendfile.
>
> It still does that as far as I can see:
>
> #if APR_HAS_SENDFILE
> core_dir_config *pdconf = ap_get_module_config(r->per_dir_config,
> &core_module);
> #endif
>
> flags = APR_READ|APR_BINARY|APR_BUFFERED;
> #if APR_HAS_SENDFILE
> flags |= ((pdconf->enable_sendfile == ENABLE_SENDFILE_OFF)
> ? 0 : APR_SENDFILE_ENABLED);
> #endif
>
> It was removed because in the absence of APR_HAS_SENDFILE, it gave a
> compiler warning. The real fix was to surround it in an ifdef, which has
> been done.
Honestly, I am -1 on any module needing knowledge of the core module
setting. We should be making the abstraction better, not pulling things
from the core config.
-Paul
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES
modules/cache/mod_cache.c modules/cache/mod_cache.h
modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
On Fri, October 27, 2006 6:05 pm, Niklas Edmundsson wrote:
> The purpose of that code was originally to make "EnableSendfile Off"
> in the config file work. APR_HAS_SENDFILE only tells you that APR has
> sendfile.
It still does that as far as I can see:
#if APR_HAS_SENDFILE
core_dir_config *pdconf = ap_get_module_config(r->per_dir_config,
&core_module);
#endif
flags = APR_READ|APR_BINARY|APR_BUFFERED;
#if APR_HAS_SENDFILE
flags |= ((pdconf->enable_sendfile == ENABLE_SENDFILE_OFF)
? 0 : APR_SENDFILE_ENABLED);
#endif
It was removed because in the absence of APR_HAS_SENDFILE, it gave a
compiler warning. The real fix was to surround it in an ifdef, which has
been done.
Regards,
Graham
--
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES
modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c
modules/cache/mod_disk_cache.h modules/cache/mod_mem_cache.c
Posted by Niklas Edmundsson <ni...@acc.umu.se>.
On Fri, 27 Oct 2006, Graham Leggett wrote:
> On Fri, October 27, 2006 4:38 pm, Davi Arnaut wrote:
>
>> Where is pdconf ? Check out all those APR_HAS_SENDFILE.
>
> Aaargh... will fix.
The purpose of that code was originally to make "EnableSendfile Off"
in the config file work. APR_HAS_SENDFILE only tells you that APR has
sendfile.
/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se | nikke@acc.umu.se
---------------------------------------------------------------------------
My favorite color? Red. No, BluAAAAAHHH!
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Re: svn commit: r468373 - in /httpd/httpd/trunk: CHANGES
modules/cache/mod_cache.c modules/cache/mod_cache.h
modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
modules/cache/mod_mem_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
On Fri, October 27, 2006 4:38 pm, Davi Arnaut wrote:
> Where is pdconf ? Check out all those APR_HAS_SENDFILE.
Aaargh... will fix.
> Those "if(" and "if (" are going on my nerves! :)
Damn, missed some.
> Swapping the metadata check would make the code much more readable:
>
> if (is_metadata)
> deal with it
> continue
>
> handle normal bucket
Cool. I also need to handle flush buckets as well.
> Err. We had the data in memory, we are going to read it back from disk
> again just in order to not block ? That's nonsense.
Yep, we have the data in memory. Now we have it on disk, and the memory
can be used for something else while the extremely slow client consumes
the download.
Don't forget, the cache is supposed to improve performance, and the
entities being cached could be large and expensive, like CGIs or mod_perl,
etc. The sooner their work is done and their memory cleared the better.
> We don't need a special bucket type!
>
> Also, are you really sure you are tracking correctly all those buckets
> with the correct offset ? The file pointer offset is unique for a single
> fd.
So far in testing with 1.5GB files, yes. Downloaded files, cached files,
original files all cmp the same.
I tried a normal file bucket type and it didn't work, as it kept closing
the underlying fd, while using the special type as contributed by Niklas
it did work. Niklas has obviously found and fixed this issue, so I didn't
pursue it further.
Regards,
Graham
--