You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Justin Erenkrantz <ju...@erenkrantz.com> on 2004/08/01 19:26:16 UTC
[PATCH] mod_cache fixes: #3
--On Sunday, August 1, 2004 11:25 AM -0400 Bill Stoddard <bi...@wstoddard.com>
wrote:
> Too many changes in one patch. Break this up into multiple consumable in 15
> minute patches and I'll review them.
* modules/experimental/mod_disk_cache.c: Allow sendfile on cache bodies.
Index: modules/experimental/mod_disk_cache.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_disk_cache.c,v
retrieving revision 1.52
diff -u -r1.52 mod_disk_cache.c
--- modules/experimental/mod_disk_cache.c 18 Mar 2004 21:40:12 -0000 1.52
+++ modules/experimental/mod_disk_cache.c 1 Aug 2004 08:24:52 -0000
@@ -370,6 +374,7 @@
cache_object_t *obj;
cache_info *info;
disk_cache_object_t *dobj;
+ int flags;
h->cache_obj = NULL;
@@ -393,14 +399,18 @@
conf->cache_root, key);
/* Open the data file */
- rc = apr_file_open(&fd, data, APR_READ|APR_BINARY, 0, r->pool);
+ flags = APR_READ|APR_BINARY;
+#ifdef APR_SENDFILE_ENABLED
+ flags |= APR_SENDFILE_ENABLED;
+#endif
+ rc = apr_file_open(&fd, data, flags, 0, r->pool);
if (rc != APR_SUCCESS) {
/* XXX: Log message */
return DECLINED;
}
/* Open the headers file */
- rc = apr_file_open(&hfd, headers, APR_READ|APR_BINARY, 0, r->pool);
+ rc = apr_file_open(&hfd, headers, flags, 0, r->pool);
if (rc != APR_SUCCESS) {
/* XXX: Log message */
return DECLINED;
Re: [PATCH] mod_cache fixes: #3
Posted by Bill Stoddard <bi...@wstoddard.com>.
Justin Erenkrantz wrote:
> --On Monday, August 2, 2004 1:05 PM -0400 Bill Stoddard
> <bi...@wstoddard.com> wrote:
>
>> I should amend my vote a -.5. The patch should work as you've coded it
>> but
>> opening a file for use with apr_sendfile causes the file to be opened for
>> overlapped i/o on Windows. I expect some of the codepaths will not
>> correctly handle this (Jeff and I fixed one such case when a file was
>> opened
>> for sendfile but mod_ssl prevented apr_sendfile from actually being
>> used).
>> Probably more bugs but they need to be found and fixed and I have no
>> idea if
>> there are other similar issues on platforms other than windows.
>
>
> Hmm. Does opening it for overlapped I/O cause a performance hit if we
> don't use sendfile later on?
No.
> I sort of dislike the fact that we even
> have to open it with the SENDFILE flag at all. But, I think asking
> non-core modules to read the core_dir_config just for this is asking too
> much. *shrug* -- justin
>
I've given this a bit more thought and I remove my objections to committing this code.
Bill
Re: [PATCH] mod_cache fixes: #3
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Monday, August 2, 2004 1:05 PM -0400 Bill Stoddard <bi...@wstoddard.com>
wrote:
> I should amend my vote a -.5. The patch should work as you've coded it but
> opening a file for use with apr_sendfile causes the file to be opened for
> overlapped i/o on Windows. I expect some of the codepaths will not
> correctly handle this (Jeff and I fixed one such case when a file was opened
> for sendfile but mod_ssl prevented apr_sendfile from actually being used).
> Probably more bugs but they need to be found and fixed and I have no idea if
> there are other similar issues on platforms other than windows.
Hmm. Does opening it for overlapped I/O cause a performance hit if we don't
use sendfile later on? I sort of dislike the fact that we even have to open
it with the SENDFILE flag at all. But, I think asking non-core modules to
read the core_dir_config just for this is asking too much. *shrug* -- justin
Re: [PATCH] mod_cache fixes: #3
Posted by Bill Stoddard <bi...@wstoddard.com>.
Justin Erenkrantz wrote:
> --On Monday, August 2, 2004 10:35 AM -0400 Bill Stoddard
> <bi...@wstoddard.com> wrote:
>
>>> * modules/experimental/mod_disk_cache.c: Allow sendfile on cache bodies.
>>>
>>
>> -1, Need to check for EnableSendfile off.
>
>
> No, core_output_filter does that check. Modules don't have that
> information whether EnableSendfile is on or not. Only the 'core' does.
> -- justin
>
I should amend my vote a -.5. The patch should work as you've coded it but opening a file for use with
apr_sendfile causes the file to be opened for overlapped i/o on Windows. I expect some of the codepaths will
not correctly handle this (Jeff and I fixed one such case when a file was opened for sendfile but mod_ssl
prevented apr_sendfile from actually being used). Probably more bugs but they need to be found and fixed and I
have no idea if there are other similar issues on platforms other than windows.
Bill
Re: [PATCH] mod_cache fixes: #3
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Monday, August 2, 2004 10:35 AM -0400 Bill Stoddard <bi...@wstoddard.com>
wrote:
>> * modules/experimental/mod_disk_cache.c: Allow sendfile on cache bodies.
>>
>
> -1, Need to check for EnableSendfile off.
No, core_output_filter does that check. Modules don't have that information
whether EnableSendfile is on or not. Only the 'core' does. -- justin
Re: [PATCH] mod_cache fixes: #3
Posted by Brian Akins <ba...@web.turner.com>.
William A. Rowe, Jr. wrote:
>
>Oh, no!
>
><Directory "/fs1/shared/www">
> EnableSendfile Off
></Directory>
>
>kills sendfile for that mount.
>
>Bill
>
>
However, in a quick handler, only the global config matters. Am I
correct? Because the per_dir has not been merged until map to storage
and Location is later.
--
Brian Akins
Senior Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_cache fixes: #3
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 10:46 AM 8/2/2004, Bill Stoddard wrote:
>EnableSendfile off is a global directive (?) so only need to check it once at startup and save it in a static variable?
Oh, no!
<Directory "/fs1/shared/www">
EnableSendfile Off
</Directory>
kills sendfile for that mount.
Bill
Re: [PATCH] mod_cache fixes: #3
Posted by Bill Stoddard <bi...@wstoddard.com>.
Brian Akins wrote:
> Bill Stoddard wrote:
>
>> /* Open the headers file */
>>
>>> - rc = apr_file_open(&hfd, headers, APR_READ|APR_BINARY, 0, r->pool);
>>> + rc = apr_file_open(&hfd, headers, flags, 0, r->pool);
>>
>>
>
> Should be something like this adapted from core:
>
> core_dir_config *core_config;
>
> core_config = (core_dir_config *) ap_get_module_config(r->per_dir_config,
> &core_module);
>
>
> if ((rv = apr_file_open(&hfd, data, APR_READ | APR_BINARY
> #if APR_HAS_SENDFILE
> |
> ((core_config->enable_sendfile ==
> ENABLE_SENDFILE_OFF)
> ? 0 : APR_SENDFILE_ENABLED)
> #endif
> , 0, r->pool)) != APR_SUCCESS)
> {
>
EnableSendfile off is a global directive (?) so only need to check it once at startup and save it in a static
variable?
Bill
Re: [PATCH] mod_cache fixes: #3
Posted by Brian Akins <ba...@web.turner.com>.
Bill Stoddard wrote:
> /* Open the headers file */
>
>> - rc = apr_file_open(&hfd, headers, APR_READ|APR_BINARY, 0, r->pool);
>> + rc = apr_file_open(&hfd, headers, flags, 0, r->pool);
>
Should be something like this adapted from core:
core_dir_config *core_config;
core_config = (core_dir_config *) ap_get_module_config(r->per_dir_config,
&core_module);
if ((rv = apr_file_open(&hfd, data, APR_READ | APR_BINARY
#if APR_HAS_SENDFILE
|
((core_config->enable_sendfile ==
ENABLE_SENDFILE_OFF)
? 0 : APR_SENDFILE_ENABLED)
#endif
, 0, r->pool)) != APR_SUCCESS)
{
--
Brian Akins
Senior Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_cache fixes: #3
Posted by Bill Stoddard <bi...@wstoddard.com>.
Justin Erenkrantz wrote:
> --On Sunday, August 1, 2004 11:25 AM -0400 Bill Stoddard
> <bi...@wstoddard.com> wrote:
>
>> Too many changes in one patch. Break this up into multiple consumable
>> in 15
>> minute patches and I'll review them.
>
>
> * modules/experimental/mod_disk_cache.c: Allow sendfile on cache bodies.
>
-1, Need to check for EnableSendfile off.
> Index: modules/experimental/mod_disk_cache.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_disk_cache.c,v
> retrieving revision 1.52
> diff -u -r1.52 mod_disk_cache.c
> --- modules/experimental/mod_disk_cache.c 18 Mar 2004 21:40:12
> -0000 1.52
> +++ modules/experimental/mod_disk_cache.c 1 Aug 2004 08:24:52 -0000
> @@ -370,6 +374,7 @@
> cache_object_t *obj;
> cache_info *info;
> disk_cache_object_t *dobj;
> + int flags;
>
> h->cache_obj = NULL;
>
> @@ -393,14 +399,18 @@
> conf->cache_root, key);
>
> /* Open the data file */
> - rc = apr_file_open(&fd, data, APR_READ|APR_BINARY, 0, r->pool);
> + flags = APR_READ|APR_BINARY;
> +#ifdef APR_SENDFILE_ENABLED
> + flags |= APR_SENDFILE_ENABLED;
> +#endif
> + rc = apr_file_open(&fd, data, flags, 0, r->pool);
> if (rc != APR_SUCCESS) {
> /* XXX: Log message */
> return DECLINED;
> }
>
> /* Open the headers file */
> - rc = apr_file_open(&hfd, headers, APR_READ|APR_BINARY, 0, r->pool);
> + rc = apr_file_open(&hfd, headers, flags, 0, r->pool);
> if (rc != APR_SUCCESS) {
> /* XXX: Log message */
> return DECLINED;
>