svn commit: r467655 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/mod

On Wed, Oct 25, 2006 at 01:44:48PM -0000, Graham Leggett
wrote:
> Author: minfrin
> Date: Wed Oct 25 06:44:47 2006
> New Revision: 467655
> 
> URL: 
http://svn.apache.org/viewvc?view=rev&rev=467655
> Log:
> mod_cache: Fix an out of memory condition that occurs
when the
> cache tries to save huge files (greater than RAM).
Buckets bigger
> than a tuneable threshold are split into smaller
buckets before
> being passed to mod_disk_cache, etc. PR 39380

Another couple of hundred lines of code and even a new
config directive, 
and this still doesn't get close to actually fixing the
problem! -1 
already, this code is just not getting better. 
mod_disk_cache is still 
liable to eat all your RAM in that apr_bucket_read() loop,
the 
apr_bucket_split() is not guaranteed to work for a morphing
bucket type.

It is simple enough to fix this problem without adding all
this code and 
without all the stuff in r450105 too, something like the
below.

Index: modules/cache/mod_cache.c
============================================================
=======
--- modules/cache/mod_cache.c	(revision 450104)
+++ modules/cache/mod_cache.c	(working copy)
 -342,6
+342,13 
         ap_set_module_config(r->request_config,
&cache_module, cache);
     }
 
+    if (!cache->tmpbb) {
+        cache->tmpbb = apr_brigade_create(r->pool,
r->connection->bucket_alloc);
+    }
+    else {
+        apr_brigade_cleanup(cache->tmpbb);
+    }
+
     reason = NULL;
     p = r->pool;
     /*
 -364,7
+371,8 
         /* pass the brigades into the cache, then pass them
          * up the filter stack
          */
-        rv =
cache->provider->store_body(cache->handle, r, in);
+        rv =
cache->provider->store_body(cache->handle, r, in,
+                                         f->next,
cache->tmpbb);
         if (rv != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_DEBUG, rv,
r->server,
                          "cache: Cache provider's
store_body failed!");
 -827,7
+835,7 
         return ap_pass_brigade(f->next, in);
     }
 
-    rv =
cache->provider->store_body(cache->handle, r, in);
+    rv =
cache->provider->store_body(cache->handle, r, in,
f->next, cache->tmpbb);
     if (rv != APR_SUCCESS) {
         ap_log_error(APLOG_MARK, APLOG_DEBUG, rv,
r->server,
                      "cache: store_body failed");
Index: modules/cache/mod_cache.h
============================================================
=======
--- modules/cache/mod_cache.h	(revision 450104)
+++ modules/cache/mod_cache.h	(working copy)
 -210,7
+210,13 
 typedef struct {
     int (*remove_entity) (cache_handle_t *h);
     apr_status_t (*store_headers)(cache_handle_t *h,
request_rec *r, cache_info *i);
-    apr_status_t (*store_body)(cache_handle_t *h,
request_rec *r, apr_bucket_brigade *b);
+    /* The store_body callback writes the contents of the
bucket
+     * brigade to the cache; if necessary any buckets may
be flushed
+     * up the filter chain by moving them to tmpbb and
passing that
+     * brigade to the f_next filter. */
+    apr_status_t (*store_body)(cache_handle_t *h,
request_rec *r, 
+                               apr_bucket_brigade *b, 
+                               ap_filter_t *f_next,
apr_bucket_brigade *tmpbb);
     apr_status_t (*recall_headers) (cache_handle_t *h,
request_rec *r);
     apr_status_t (*recall_body) (cache_handle_t *h,
apr_pool_t *p, apr_bucket_brigade *bb); 
     int (*create_entity) (cache_handle_t *h, request_rec
*r,
 -246,6
+252,7 
     apr_time_t lastmod;                 /* last-modified
time */
     cache_info *info;                   /* current cache
info */
     ap_filter_t *remove_url_filter;     /* Enable us to
remove the filter */
+    apr_bucket_brigade *tmpbb;          /* Temporary bucket
brigade. */
 } cache_request_rec;
 
 
Index: modules/cache/mod_disk_cache.c
============================================================
=======
--- modules/cache/mod_disk_cache.c	(revision 450104)
+++ modules/cache/mod_disk_cache.c	(working copy)
 -56,7
+56,6 
 /* Forward declarations */
 static int remove_entity(cache_handle_t *h);
 static apr_status_t store_headers(cache_handle_t *h,
request_rec *r, cache_info *i);
-static apr_status_t store_body(cache_handle_t *h,
request_rec *r, apr_bucket_brigade *b);
 static apr_status_t recall_headers(cache_handle_t *h,
request_rec *r);
 static apr_status_t recall_body(cache_handle_t *h,
apr_pool_t *p, apr_bucket_brigade *bb);
 static apr_status_t read_array(request_rec *r,
apr_array_header_t* arr,
 -977,9
+976,10 
 }
 
 static apr_status_t store_body(cache_handle_t *h,
request_rec *r,
-                               apr_bucket_brigade *bb)
+                               apr_bucket_brigade *bb,
+                               ap_filter_t *f_next,
apr_bucket_brigade *tmpbb)
 {
-    apr_bucket *e;
+    apr_bucket *e, *next;
     apr_status_t rv;
     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,
 -998,12
+998,16 
         dobj->file_size = 0;
     }
 
-    for (e = APR_BRIGADE_FIRST(bb);
-         e != APR_BRIGADE_SENTINEL(bb);
-         e = APR_BUCKET_NEXT(e))
-    {
+    e = APR_BRIGADE_FIRST(bb);
+    while (e != APR_BRIGADE_SENTINEL(bb) &&
!APR_BUCKET_IS_EOS(e)) {
         const char *str;
         apr_size_t length, written;
+
+        if (APR_BUCKET_IS_METADATA(e)) {
+            e = APR_BUCKET_NEXT(e);
+            continue;
+        }
+
         rv = apr_bucket_read(e, &str, &length,
APR_BLOCK_READ);
         if (rv != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_ERR, 0,
r->server,
 -1032,12
+1036,33 
             file_cache_errorcleanup(dobj, r);
             return APR_EGENERAL;
         }
+
+        next = APR_BUCKET_NEXT(e);
+
+        /* Move the bucket to the temp brigade and flush it
up the
+         * filter chain. */
+        APR_BUCKET_REMOVE(e);
+        APR_BRIGADE_INSERT_HEAD(tmpbb, e);
+        rv = ap_pass_brigade(f_next, tmpbb);
+        if (rv) {
+            ap_log_error(APLOG_MARK, APLOG_DEBUG, rv,
r->server,
+                         "disk_cache: failed to flush
brigade for URL %s",
+                         h->cache_obj->key);
+            /* Remove the intermediate cache file and
return non-APR_SUCCESS */
+            file_cache_errorcleanup(dobj, r);
+            return rv;
+        }
+        
+        /* Discard the bucket and move on. */
+        apr_brigade_cleanup(tmpbb);
+
+        e = next;
     }
 
     /* Was this the final bucket? If yes, close the temp
file and perform
      * sanity checks.
      */
-    if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
+    if (e != APR_BRIGADE_SENTINEL(bb) &&
APR_BUCKET_IS_EOS(e)) {
         if (r->connection->aborted || r->no_cache)
{
             ap_log_error(APLOG_MARK, APLOG_INFO, 0,
r->server,
                          "disk_cache: Discarding body
for URL %s "

Joe Orton wrote:
> On Wed, Oct 25, 2006 at 01:44:48PM -0000, Graham
Leggett wrote:
>> Author: minfrin
>> Date: Wed Oct 25 06:44:47 2006
>> New Revision: 467655
>>
>> URL: 
http://svn.apache.org/viewvc?view=rev&rev=467655
>> Log:
>> mod_cache: Fix an out of memory condition that
occurs when the
>> cache tries to save huge files (greater than RAM).
Buckets bigger
>> than a tuneable threshold are split into smaller
buckets before
>> being passed to mod_disk_cache, etc. PR 39380
> 
> Another couple of hundred lines of code and even a new
config directive, 
> and this still doesn't get close to actually fixing the
problem! -1 
> already, this code is just not getting better. 
mod_disk_cache is still 
> liable to eat all your RAM in that apr_bucket_read()
loop, the 
> apr_bucket_split() is not guaranteed to work for a
morphing bucket type.
> 
> It is simple enough to fix this problem without adding
all this code and 
> without all the stuff in r450105 too, something like
the below.
> 

And you almost got it right. We don't want to stop caching
if the
client's connection fail and we must not slow the caching
because of a
slow client (that's why I didn't pass the brigade).

In the end, your's do almost the same as mine [1] expect
that I dint's
pass the new buckets up the chain before deleting then (and
it was file
bucket exclusive). Copying to disk tends to be faster then
sending to a
client.

--
Davi Arnaut

1 http://marc.theaimsgroup.com/?l=ap
ache-httpd-dev&m=115971884005419&w=2

On Wed, October 25, 2006 5:58 pm, Joe Orton wrote:

> Another couple of hundred lines of code and even a new
config directive,
> and this still doesn't get close to actually fixing the
problem! -1
> already, this code is just not getting better.

As has been explained a few times before, there isn't
"a problem" or "the
problem", but rather many problems.

These problems, are only practically solveable with many
patches, each one
addressing a specific problem or behavior.

The patch just committed removes a burden from the cache
providers, in
that from a single source, there is control over the size of
buckets that
cache providers are expected to handle. The alternative was
one directive
per cache provider, which is not ideal.

The patch just committed is part of an ongoing work to
improve the
behaviour of the cache in the real world, to solve real
problems at real
sites.

Progress to date:

- The cache can now cache a file, and serve from the cache
at the same time.

- While caching a file, data is sent both to the cache, and
to the end
client, at the same time. The cache no longer caches the
entire file
before sending it to the client.

- It is possible to cache a file at full disk speed, even
when the
downstream client is slower than the disk, without buffering
data in RAM.

Next step is to remove the copy_body() hack inside
mod_disk_cache, as it
is unnecessary.

To say "it's not getting better" without actually
running the code and
seeing the progress involved is very hollow criticism.

I appreciate the effort involved in pointing out areas where
issues need
to be addressed, but having to contend with the constant
barrage of
negativity, and the ridiculous notion that "the problem
cannot be solved",
is a really tiring exercise.

> mod_disk_cache is still
> liable to eat all your RAM in that apr_bucket_read()
loop,

Correct, and this will be fixed.

We ideally want file writes to happen using a file write
output filter,
which can then encapsulate any bucket specific weirdness
exactly like
ap_core_output_filter does.

> the
> apr_bucket_split() is not guaranteed to work for a
morphing bucket type.

Then morphing buckets are broken.

The split method must either succeed, or return a non
success APR value.
Both of these cases are handled for by the split loop. If
morphing buckets
crash, then they must be fixed.

> It is simple enough to fix this problem without adding
all this code and
> without all the stuff in r450105 too, something like
the below.

I still see that this call is intact:

apr_bucket_read(e, &str, &length, APR_BLOCK_READ)

Given a 4.7GB bucket, it will attempt to read all 4.7GB of
the bucket into
RAM, and abort.

Is there something I am missing?

Regards,
Graham
--

Joe Orton wrote:
> On Wed, Oct 25, 2006 at 01:44:48PM -0000, Graham
Leggett wrote:
>> Author: minfrin
>> Date: Wed Oct 25 06:44:47 2006
>> New Revision: 467655
>>
>> URL: 
http://svn.apache.org/viewvc?view=rev&rev=467655
>> Log:
>> mod_cache: Fix an out of memory condition that
occurs when the
>> cache tries to save huge files (greater than RAM).
Buckets bigger
>> than a tuneable threshold are split into smaller
buckets before
>> being passed to mod_disk_cache, etc. PR 39380
> 
> Another couple of hundred lines of code and even a new
config directive, 
> and this still doesn't get close to actually fixing the
problem! -1 
> already, this code is just not getting better. 
mod_disk_cache is still 
> liable to eat all your RAM in that apr_bucket_read()
loop, the 
> apr_bucket_split() is not guaranteed to work for a
morphing bucket type.
> 
> It is simple enough to fix this problem without adding
all this code and 
> without all the stuff in r450105 too, something like
the below.
> 

And you almost got it right. We don't want to stop caching
if the
client's connection fail and we must not slow the caching
because of a
slow client (that's why I didn't pass the brigade).

In the end, your's do almost the same as mine [1] expect
that I dint's
pass the new buckets up the chain before deleting then (and
it was file
bucket exclusive). Copying to disk tends to be faster then
sending to a
client.

--
Davi Arnaut

1 http://marc.theaimsgroup.com/?l=ap
ache-httpd-dev&m=115971884005419&w=2

On Wed, October 25, 2006 5:58 pm, Joe Orton wrote:

> Another couple of hundred lines of code and even a new
config directive,
> and this still doesn't get close to actually fixing the
problem! -1
> already, this code is just not getting better.

As has been explained a few times before, there isn't
"a problem" or "the
problem", but rather many problems.

These problems, are only practically solveable with many
patches, each one
addressing a specific problem or behavior.

The patch just committed removes a burden from the cache
providers, in
that from a single source, there is control over the size of
buckets that
cache providers are expected to handle. The alternative was
one directive
per cache provider, which is not ideal.

The patch just committed is part of an ongoing work to
improve the
behaviour of the cache in the real world, to solve real
problems at real
sites.

Progress to date:

- The cache can now cache a file, and serve from the cache
at the same time.

- While caching a file, data is sent both to the cache, and
to the end
client, at the same time. The cache no longer caches the
entire file
before sending it to the client.

- It is possible to cache a file at full disk speed, even
when the
downstream client is slower than the disk, without buffering
data in RAM.

Next step is to remove the copy_body() hack inside
mod_disk_cache, as it
is unnecessary.

To say "it's not getting better" without actually
running the code and
seeing the progress involved is very hollow criticism.

I appreciate the effort involved in pointing out areas where
issues need
to be addressed, but having to contend with the constant
barrage of
negativity, and the ridiculous notion that "the problem
cannot be solved",
is a really tiring exercise.

> mod_disk_cache is still
> liable to eat all your RAM in that apr_bucket_read()
loop,

Correct, and this will be fixed.

We ideally want file writes to happen using a file write
output filter,
which can then encapsulate any bucket specific weirdness
exactly like
ap_core_output_filter does.

> the
> apr_bucket_split() is not guaranteed to work for a
morphing bucket type.

Then morphing buckets are broken.

The split method must either succeed, or return a non
success APR value.
Both of these cases are handled for by the split loop. If
morphing buckets
crash, then they must be fixed.

> It is simple enough to fix this problem without adding
all this code and
> without all the stuff in r450105 too, something like
the below.

I still see that this call is intact:

apr_bucket_read(e, &str, &length, APR_BLOCK_READ)

Given a 4.7GB bucket, it will attempt to read all 4.7GB of
the bucket into
RAM, and abort.

Is there something I am missing?

Regards,
Graham
--

On Wed, Oct 25, 2006 at 01:54:04PM -0300, Davi Arnaut wrote:
> Joe Orton wrote:
> > Another couple of hundred lines of code and even a
new config directive, 
> > and this still doesn't get close to actually
fixing the problem! -1 
> > already, this code is just not getting better. 
mod_disk_cache is still 
> > liable to eat all your RAM in that
apr_bucket_read() loop, the 
> > apr_bucket_split() is not guaranteed to work for a
morphing bucket type.
> > 
> > It is simple enough to fix this problem without
adding all this code and 
> > without all the stuff in r450105 too, something
like the below.
> > 
> 
> And you almost got it right. We don't want to stop
caching if the
> client's connection fail

...a simple enough change.

> and we must not slow the caching because of a slow
client (that's why 
> I didn't pass the brigade).

There is no other acceptable solution AFAICS.  Buffering the
entire 
brigade (either to disk, or into RAM as the current code
does) before 
writing to the client is not OK, polling on buckets is not
possible, 
using threads is not OK, using non-blocking writes up the
output filter 
chain is not possible.  Any other ideas?

Regards,

joe

Graham Leggett wrote:
> On Wed, October 25, 2006 5:58 pm, Joe Orton wrote:
> 
>> Another couple of hundred lines of code and even a
new config directive,
>> and this still doesn't get close to actually fixing
the problem! -1
>> already, this code is just not getting better.
> 
> As has been explained a few times before, there isn't
"a problem" or "the
> problem", but rather many problems.
> 
> These problems, are only practically solveable with
many patches, each one
> addressing a specific problem or behavior.

Well said.

> The patch just committed removes a burden from the
cache providers, in
> that from a single source, there is control over the
size of buckets that
> cache providers are expected to handle. The alternative
was one directive
> per cache provider, which is not ideal.
> 
> The patch just committed is part of an ongoing work to
improve the
> behaviour of the cache in the real world, to solve real
problems at real
> sites.
> 
> Progress to date:
> 
> - The cache can now cache a file, and serve from the
cache at the same time.
> 
> - While caching a file, data is sent both to the cache,
and to the end
> client, at the same time. The cache no longer caches
the entire file
> before sending it to the client.
> 
> - It is possible to cache a file at full disk speed,
even when the
> downstream client is slower than the disk, without
buffering data in RAM.
> 
> Next step is to remove the copy_body() hack inside
mod_disk_cache, as it
> is unnecessary.

I would (I know I can't  vote for
reverting that last patches (up to
r450089) so that we can start all over.

> To say "it's not getting better" without
actually running the code and
> seeing the progress involved is very hollow criticism.
> 
> I appreciate the effort involved in pointing out areas
where issues need
> to be addressed, but having to contend with the
constant barrage of
> negativity, and the ridiculous notion that "the
problem cannot be solved",
> is a really tiring exercise.

+1

>> mod_disk_cache is still
>> liable to eat all your RAM in that
apr_bucket_read() loop,
> 
> Correct, and this will be fixed.
> 
> We ideally want file writes to happen using a file
write output filter,
> which can then encapsulate any bucket specific
weirdness exactly like
> ap_core_output_filter does.
> 
>> the
>> apr_bucket_split() is not guaranteed to work for a
morphing bucket type.
> 
> Then morphing buckets are broken.
> 
> The split method must either succeed, or return a non
success APR value.
> Both of these cases are handled for by the split loop.
If morphing buckets
> crash, then they must be fixed.
> 
>> It is simple enough to fix this problem without
adding all this code and
>> without all the stuff in r450105 too, something
like the below.
> 
> I still see that this call is intact:
> 
> apr_bucket_read(e, &str, &length,
APR_BLOCK_READ)
> 
> Given a 4.7GB bucket, it will attempt to read all 4.7GB
of the bucket into
> RAM, and abort.
> 
> Is there something I am missing?

Yes, first because size_t is 32 bits . When you
do a read like this on
a file bucket, the whole bucket is not read into memory. The
read
function splits the bucket and changes the current bucket to
refer to
data that was read.

The problem lies that those new buckets keep accumulating in
the
brigade! See my patch again.

So Joe's patch removes this newly implicitly created bucket
from the
brigade, passes it to the client, and delete it.

--
Davi Arnaut

Joe Orton wrote:
> On Wed, Oct 25, 2006 at 01:54:04PM -0300, Davi Arnaut
wrote:
>> Joe Orton wrote:
>>> Another couple of hundred lines of code and
even a new config directive, 
>>> and this still doesn't get close to actually
fixing the problem! -1 
>>> already, this code is just not getting better. 
mod_disk_cache is still 
>>> liable to eat all your RAM in that
apr_bucket_read() loop, the 
>>> apr_bucket_split() is not guaranteed to work
for a morphing bucket type.
>>>
>>> It is simple enough to fix this problem without
adding all this code and 
>>> without all the stuff in r450105 too, something
like the below.
>>>
>> And you almost got it right. We don't want to stop
caching if the
>> client's connection fail
> 
> ...a simple enough change.

Yes.

>> and we must not slow the caching because of a slow
client (that's why 
>> I didn't pass the brigade).
> 
> There is no other acceptable solution AFAICS. 
Buffering the entire 
> brigade (either to disk, or into RAM as the current
code does) before 
> writing to the client is not OK, polling on buckets is
not possible, 
> using threads is not OK, using non-blocking writes up
the output filter 
> chain is not possible.  Any other ideas?

I think we should build the necessary infrastructure to
solve this
problem once and for all. Be it either non-blocking writes,
AIO or add
support to the core output filter to write data to different
destinations.

--
Davi Arnaut

Davi Arnaut wrote:
> Joe Orton wrote:
>>> and we must not slow the caching because of a
slow client (that's why 
>>> I didn't pass the brigade).
>> There is no other acceptable solution AFAICS. 
Buffering the entire 
>> brigade (either to disk, or into RAM as the current
code does) before 
>> writing to the client is not OK, polling on buckets
is not possible, 
>> using threads is not OK, using non-blocking writes
up the output filter 
>> chain is not possible.  Any other ideas?
> 
> I think we should build the necessary infrastructure to
solve this
> problem once and for all. Be it either non-blocking
writes, AIO or add
> support to the core output filter to write data to
different destinations.

Thats not going to be possible until 3.0. Good luck 

-Paul

Paul Querna wrote:
> 
> Thats not going to be possible until 3.0. Good luck


Paul's right.  The next expectiation we break, that a
protocol spends it's
life on a single thread.  That is really a 3.0 magnitude
change because it
will break modules in a major way.

Then, the final expectation to break is that a -request-
spends it's life on
a given thread.  That's earth shattering to developers, and
really is beyond
the 3.0 release (if we want to see 3.0 adopted in the next
few years).