On Thu, Feb 19, 2009 at 10:00:50PM +0100, Ruediger Pluem wrote:
> On 02/19/2009 12:32 PM, Joe Orton wrote:
...
> > @@ -497,13 +500,17 @@
> > next = APR_BUCKET_NEXT(bucket);
> > }
> > bytes_in_brigade += bucket->length;
> > - if (!APR_BUCKET_IS_FILE(bucket)) {
> > + if (APR_BUCKET_IS_FILE(bucket)) {
> > + num_files_in_brigade++;
> > + }
> > + else {
> > non_file_bytes_in_brigade += bucket->length;
> > }
> > }
> > }
> >
> > - if (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER) {
> > + if (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER
> > + || num_files_in_brigade >= THRESHOLD_MAX_FILES) {
>
> If the 16 FD's were split over more then one brigade and the
> brigades before us were set aside the FD's belong already to the wrong pool
> (the connection pool). Deleting a file bucket doesn't close the FD it uses.
Not sure what the concern is there - this loop is iterating over the
concatenation of the buffered brigade an the "new" brigade (right?), so
it will count the total number of buckets which are potentially left
buffered after this c_o_f invocation terminates.
w.r.t. MMAP buckets: the 64K bytes limit will apply here, since FILE
only morphs into MMAP if the file size is > 8K. (And no, theoretically,
the fd from which an MMAP bucket was derived is not needed after the
mmap() call, but I don't think the fds actually get closed earlier than
pool closure, normally)
w.r.t. multiple FILE buckets for a single fd; failing to buffer as much
as possible will not be the end of the world, but I suppose that case is
common, so we could cope with it something like this. (completely
untested)
Index: server/core_filters.c
===================================================================
--- server/core_filters.c (revision 734690)
+++ server/core_filters.c (working copy)
@@ -367,6 +367,7 @@
#define THRESHOLD_MIN_WRITE 4096
#define THRESHOLD_MAX_BUFFER 65536
+#define THRESHOLD_MAX_FILES 16
/* Optional function coming from mod_logio, used for logging of output
* traffic
@@ -380,7 +381,8 @@
core_output_filter_ctx_t *ctx = net->out_ctx;
apr_bucket_brigade *bb;
apr_bucket *bucket, *next;
- apr_size_t bytes_in_brigade, non_file_bytes_in_brigade;
+ apr_size_t bytes_in_brigade, non_file_bytes_in_brigade,
num_files_in_brigade;
+ apr_file_t *last_file_seen;
/* Fail quickly if the connection has already been aborted. */
if (c->aborted) {
@@ -466,6 +468,9 @@
bytes_in_brigade = 0;
non_file_bytes_in_brigade = 0;
+ num_files_in_brigade = 0;
+ last_file_seen = NULL;
+
for (bucket = APR_BRIGADE_FIRST(bb); bucket != APR_BRIGADE_SENTINEL(bb);
bucket = next) {
next = APR_BUCKET_NEXT(bucket);
@@ -497,13 +502,20 @@
next = APR_BUCKET_NEXT(bucket);
}
bytes_in_brigade += bucket->length;
- if (!APR_BUCKET_IS_FILE(bucket)) {
+ if (APR_BUCKET_IS_FILE(bucket)
+ && (last_file_seen == NULL
+ || last_file_seen != ((apr_bucket_file
*)bucket->data)->fd)) {
+ num_files_in_brigade++;
+ last_file_seen = ((apr_bucket_file *)bucket->data)->fd;
+ }
+ else {
non_file_bytes_in_brigade += bucket->length;
}
}
}
- if (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER) {
+ if (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER
+ || num_files_in_brigade >= THRESHOLD_MAX_FILES) {
/* ### Writing the entire brigade may be excessive; we really just
* ### need to send enough data to be under THRESHOLD_MAX_BUFFER.
*/
