You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Greg Ames <gr...@apache.org> on 2003/02/06 22:29:38 UTC
[PATCH] Sendfile API compatibility breakage
Between 2.0.43 and 2.0.44, the performance of SPECweb99 standard dynamic GETs
has degraded about 7%. Static files are fine, and ab benchmarks of small
dynamic requests are also fine. oprofile + strace shows that the difference
with the SPEC dynamic requests is that sendfile is no longer used in .44.
The problem is caused by the APR_SENDFILE_ENABLED flag. If your
generator/handler does not explicitly set that flag at apr_file_open() time, the
core_output_filter won't use sendfile in .44 or beyond, violating the principle
of least astonishment.
The following patch restores API compatibility for non-core modules. It
reverses the polarity of the flag so that you get the old behavior unless your
module takes explicit action to invoke the new behavior. The EnableSendfile
On/Off directive still works as documented, and SPECweb99 performance is back to
where it should be.
Greg
Re: [PATCH] Sendfile API compatibility breakage
Posted by Greg Ames <gr...@apache.org>.
William A. Rowe, Jr. wrote:
> At 02:51 PM 2/12/2003, Greg Ames wrote:
>
>
>>I think there should be an MMN bump in the 2.1 stream for this,
>
> It's a new feature. +1 on an MMN *minor* bump to 2.0.45-dev and the 2.1 tree.
If we bump the MMN in both streams, then the simplest fix is probably to leave
the core as-is and patch mod_specweb99 to set the APR_SENDFILE_ENABLED bit
wrapped in an appropriate MMN incantation
> The new feature doesn't *break* anybody by this schema, correct?
We should be cool either way.
Greg
Re: [PATCH] Sendfile API compatibility breakage
Posted by Greg Ames <gr...@apache.org>.
William A. Rowe, Jr. wrote:
> At 02:51 PM 2/12/2003, Greg Ames wrote:
>
>
>>I think there should be an MMN bump in the 2.1 stream for this,
>
> It's a new feature. +1 on an MMN *minor* bump to 2.0.45-dev and the 2.1 tree.
If we bump the MMN in both streams, then the simplest fix is probably to leave
the core as-is and patch mod_specweb99 to set the APR_SENDFILE_ENABLED bit
wrapped in an appropriate MMN incantation
> The new feature doesn't *break* anybody by this schema, correct?
We should be cool either way.
Greg
Re: [PATCH] Sendfile API compatibility breakage
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 02:51 PM 2/12/2003, Greg Ames wrote:
>I think there should be an MMN bump in the 2.1 stream for this, so independent modules can use the new API if they chose to. In 2.0 stable, IMO we should do our best to make it transparent.
It's a new feature. +1 on an MMN *minor* bump to 2.0.45-dev and the 2.1 tree.
The new feature doesn't *break* anybody by this schema, correct? If we (the
APR team) shut down sendfile on some obtuse or buggy platform, that's our
(wise) choice to help users avoid hitting such bugs.
Bill
Re: [PATCH] Sendfile API compatibility breakage
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 02:51 PM 2/12/2003, Greg Ames wrote:
>I think there should be an MMN bump in the 2.1 stream for this, so independent modules can use the new API if they chose to. In 2.0 stable, IMO we should do our best to make it transparent.
It's a new feature. +1 on an MMN *minor* bump to 2.0.45-dev and the 2.1 tree.
The new feature doesn't *break* anybody by this schema, correct? If we (the
APR team) shut down sendfile on some obtuse or buggy platform, that's our
(wise) choice to help users avoid hitting such bugs.
Bill
Re: [PATCH] Sendfile API compatibility breakage
Posted by Greg Ames <gr...@apache.org>.
William A. Rowe, Jr. wrote:
> I suggest the following as a friendly compromise to avoid voting...
this sounds like progress.
> introduce DISABLE, don't drop ENABLE (especially since your patch would *reverse*
> the meaning of the EnableSendfile directive for httpd 2.0.44 built against
> a newer apr!!!)
D'oh! You're right, good catch.
> Respect both flags unless both are passed to apr_file_open.
>
> If neither is specified, let the platform author of the apr_sendfile() code decide
> how *safe* it is to enable the feature. This could include context (such as
> the filesystem if they want to query it) or filesize (<2gb) or whatever choices
> are wise.
>
> ENABLE and DISABLE would both be explicit overrides to any sanity tests.
> Based on current bugzilla reports, httpd EnableSendfile should gain the
> 'default' state. So we could turn it on or off, or leave it up to APR.
I think this could be made to work. I assume the platform author makes the call
on what to do with sendfile in the default case at apr_file_open time.
Otherwise there's a lot of code that would have to be churned between httpd and
apr without much benefit.
I think there should be an MMN bump in the 2.1 stream for this, so independent
modules can use the new API if they chose to. In 2.0 stable, IMO we should do
our best to make it transparent.
Greg
Re: [PATCH] Sendfile API compatibility breakage
Posted by Greg Ames <gr...@apache.org>.
William A. Rowe, Jr. wrote:
> I suggest the following as a friendly compromise to avoid voting...
this sounds like progress.
> introduce DISABLE, don't drop ENABLE (especially since your patch would *reverse*
> the meaning of the EnableSendfile directive for httpd 2.0.44 built against
> a newer apr!!!)
D'oh! You're right, good catch.
> Respect both flags unless both are passed to apr_file_open.
>
> If neither is specified, let the platform author of the apr_sendfile() code decide
> how *safe* it is to enable the feature. This could include context (such as
> the filesystem if they want to query it) or filesize (<2gb) or whatever choices
> are wise.
>
> ENABLE and DISABLE would both be explicit overrides to any sanity tests.
> Based on current bugzilla reports, httpd EnableSendfile should gain the
> 'default' state. So we could turn it on or off, or leave it up to APR.
I think this could be made to work. I assume the platform author makes the call
on what to do with sendfile in the default case at apr_file_open time.
Otherwise there's a lot of code that would have to be churned between httpd and
apr without much benefit.
I think there should be an MMN bump in the 2.1 stream for this, so independent
modules can use the new API if they chose to. In 2.0 stable, IMO we should do
our best to make it transparent.
Greg
Re: [PATCH] Sendfile API compatibility breakage
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 12:54 PM 2/11/2003, you wrote:
>William A. Rowe, Jr. wrote:
>>I'm very close to an outright veto of such a change, at this time...
>
>forget it.
>
>Not only is the binary interface to sendfile broken now, the source interface is broken too. Since there was no mmn bump, there is no reasonable way a module which ships independently from the core can write code to enable sendfile with the new interface and expect the module to compile against 2.0.43 or earlier.
>
>I like the concept of disabling sendfile at runtime to handle the corner cases like nfs and broken OSes. No argument there. But the existing code goes beyond that. As things stand now, non-core module owners are screwed if they want to use sendfile in the majority of cases where it works fine.
I suggest the following as a friendly compromise to avoid voting... introduce
DISABLE, don't drop ENABLE (especially since your patch would *reverse*
the meaning of the EnableSendfile directive for httpd 2.0.44 built against
a newer apr!!!) Respect both flags unless both are passed to apr_file_open.
If neither is specified, let the platform author of the apr_sendfile() code decide
how *safe* it is to enable the feature. This could include context (such as
the filesystem if they want to query it) or filesize (<2gb) or whatever choices
are wise.
ENABLE and DISABLE would both be explicit overrides to any sanity tests.
Based on current bugzilla reports, httpd EnableSendfile should gain the
'default' state. So we could turn it on or off, or leave it up to APR.
Bill
Re: [PATCH] Sendfile API compatibility breakage
Posted by Greg Ames <gr...@apache.org>.
William A. Rowe, Jr. wrote:
> I'm very close to an outright veto of such a change, at this time...
forget it.
Not only is the binary interface to sendfile broken now, the source interface is
broken too. Since there was no mmn bump, there is no reasonable way a module
which ships independently from the core can write code to enable sendfile with
the new interface and expect the module to compile against 2.0.43 or earlier.
I like the concept of disabling sendfile at runtime to handle the corner cases
like nfs and broken OSes. No argument there. But the existing code goes beyond
that. As things stand now, non-core module owners are screwed if they want to
use sendfile in the majority of cases where it works fine.
Greg
Re: [PATCH] Sendfile API compatibility breakage
Posted by Greg Ames <gr...@apache.org>.
William A. Rowe, Jr. wrote:
> I'm very close to an outright veto of such a change, at this time...
forget it.
Not only is the binary interface to sendfile broken now, the source interface is
broken too. Since there was no mmn bump, there is no reasonable way a module
which ships independently from the core can write code to enable sendfile with
the new interface and expect the module to compile against 2.0.43 or earlier.
I like the concept of disabling sendfile at runtime to handle the corner cases
like nfs and broken OSes. No argument there. But the existing code goes beyond
that. As things stand now, non-core module owners are screwed if they want to
use sendfile in the majority of cases where it works fine.
Greg
Re: [PATCH] Sendfile API compatibility breakage
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
I'm very close to an outright veto of such a change, at this time...
My reason is pretty simple... a good chunk of bugzilla reports have revolved
around one broken sendfile implementation or another. This isn't specific to
a small handful of platforms. It is a problem across various nfs and other
filesystems, and specific kernel levels. And other observers have pointed
out other good reasons when not to use it.
I'd prefer we find and fix the root bug instead of enabling sendfile liberally.
The caller really needs to understand exactly what they are doing before
shipping an application that uses our sendfile support. Until that support
is alot more robust or deterministic, this patch isn't healthy.
Your patch assumes that the caller knows that sendfile isn't healthy. It makes
more sense for the caller to assert that sendfile *is* known to be healthy.
If the caller doesn't say - and we are 100% certain that sendfile on a given
architecture/filesystem is absolutely fine, then *we* should go ahead and
enable it ourselves in the apr_file_open() code. If that means checking the
mount then we are in for some hassle.
If you want to argue for "violating the principle of least astonishment", again
I'm asserting that corrupted transmission is the greater astonishment :-)
No caller within httpd should be allowed to use this API (just yet) until they
resepect the users choice of disabling sendfile (violating the choice of *not*
using sendfile does much more harm than violating a choice to use it.)
Bill
At 03:29 PM 2/6/2003, Greg Ames wrote:
>Between 2.0.43 and 2.0.44, the performance of SPECweb99 standard dynamic GETs has degraded about 7%. Static files are fine, and ab benchmarks of small dynamic requests are also fine. oprofile + strace shows that the difference with the SPEC dynamic requests is that sendfile is no longer used in .44.
>
>The problem is caused by the APR_SENDFILE_ENABLED flag. If your generator/handler does not explicitly set that flag at apr_file_open() time, the core_output_filter won't use sendfile in .44 or beyond, violating the principle of least astonishment.
>
>The following patch restores API compatibility for non-core modules. It reverses the polarity of the flag so that you get the old behavior unless your module takes explicit action to invoke the new behavior. The EnableSendfile On/Off directive still works as documented, and SPECweb99 performance is back to where it should be.
>
>Greg
>
>
>Index: modules/experimental/mod_mem_cache.c
>===================================================================
>RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_mem_cache.c,v
>retrieving revision 1.92
>diff -u -d -b -r1.92 mod_mem_cache.c
>--- modules/experimental/mod_mem_cache.c 3 Feb 2003 17:53:00 -0000 1.92
>+++ modules/experimental/mod_mem_cache.c 5 Feb 2003 22:36:01 -0000
>@@ -942,7 +942,7 @@
> const char *name;
> /* Open a new XTHREAD handle to the file */
> apr_file_name_get(&name, file);
>- mobj->flags = ((APR_SENDFILE_ENABLED & apr_file_flags_get(file))
>+ mobj->flags = ((APR_SENDFILE_DISABLED & apr_file_flags_get(file))
> | APR_READ | APR_BINARY | APR_XTHREAD | APR_FILE_NOCLEANUP);
> rv = apr_file_open(&tmpfile, name, mobj->flags,
> APR_OS_DEFAULT, r->pool);
>Index: server/core.c
>===================================================================
>RCS file: /home/cvs/httpd-2.0/server/core.c,v
>retrieving revision 1.231
>diff -u -d -b -r1.231 core.c
>--- server/core.c 3 Feb 2003 17:53:18 -0000 1.231
>+++ server/core.c 5 Feb 2003 22:36:01 -0000
>@@ -3319,7 +3319,7 @@
> if ((status = apr_file_open(&fd, r->filename, APR_READ | APR_BINARY
> #if APR_HAS_SENDFILE
> | ((d->enable_sendfile == ENABLE_SENDFILE_OFF)
>- ? 0 : APR_SENDFILE_ENABLED)
>+ ? APR_SENDFILE_DISABLED : 0)
> #endif
> , 0, r->pool)) != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r,
>@@ -3949,7 +3949,7 @@
> }
>
> #if APR_HAS_SENDFILE
>- if (apr_file_flags_get(fd) & APR_SENDFILE_ENABLED) {
>+ if (!(apr_file_flags_get(fd) & APR_SENDFILE_DISABLED)) {
>
> if (c->keepalive == AP_CONN_CLOSE && APR_BUCKET_IS_EOS(last_e)) {
> /* Prepare the socket to be reused */
>Index: srclib/apr/file_io/win32/open.c
>===================================================================
>RCS file: /home/cvs/apr/file_io/win32/open.c,v
>retrieving revision 1.117
>diff -u -d -b -r1.117 open.c
>--- srclib/apr/file_io/win32/open.c 7 Jan 2003 00:52:54 -0000 1.117
>+++ srclib/apr/file_io/win32/open.c 5 Feb 2003 22:36:01 -0000
>@@ -374,7 +374,7 @@
> {
> apr_wchar_t wfname[APR_PATH_MAX];
>
>- if (flag & APR_SENDFILE_ENABLED) {
>+ if (!(flag & APR_SENDFILE_DISABLED)) {
> /* This feature is required to enable sendfile operations
> * against the file on Win32. Also implies APR_XTHREAD.
> */
>@@ -393,11 +393,10 @@
> ELSE_WIN_OS_IS_ANSI {
> handle = CreateFileA(fname, oflags, sharemode,
> NULL, createflags, attributes, 0);
>- if (flag & APR_SENDFILE_ENABLED) {
>- /* This feature is not supported on this platform.
>+ /* sendfile is not supported on this platform.
> */
>- flag &= ~APR_SENDFILE_ENABLED;
>- }
>+ flag |= APR_SENDFILE_DISABLED;
>+
>
> }
> #endif
>Index: srclib/apr/include/apr_file_io.h
>===================================================================
>RCS file: /home/cvs/apr/include/apr_file_io.h,v
>retrieving revision 1.136
>diff -u -d -b -r1.136 apr_file_io.h
>--- srclib/apr/include/apr_file_io.h 24 Jan 2003 19:24:24 -0000 1.136
>+++ srclib/apr/include/apr_file_io.h 5 Feb 2003 22:36:01 -0000
>@@ -103,7 +103,7 @@
> writes across process/machines */
> #define APR_FILE_NOCLEANUP 2048 /**< Do not register a cleanup when the file
> is opened */
>-#define APR_SENDFILE_ENABLED 4096 /**< Advisory flag that this file should
>+#define APR_SENDFILE_DISABLED 4096 /**< Advisory flag that this file does not
> support apr_sendfile operation */
> /** @} */
>
Re: [PATCH] Sendfile API compatibility breakage
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
I'm very close to an outright veto of such a change, at this time...
My reason is pretty simple... a good chunk of bugzilla reports have revolved
around one broken sendfile implementation or another. This isn't specific to
a small handful of platforms. It is a problem across various nfs and other
filesystems, and specific kernel levels. And other observers have pointed
out other good reasons when not to use it.
I'd prefer we find and fix the root bug instead of enabling sendfile liberally.
The caller really needs to understand exactly what they are doing before
shipping an application that uses our sendfile support. Until that support
is alot more robust or deterministic, this patch isn't healthy.
Your patch assumes that the caller knows that sendfile isn't healthy. It makes
more sense for the caller to assert that sendfile *is* known to be healthy.
If the caller doesn't say - and we are 100% certain that sendfile on a given
architecture/filesystem is absolutely fine, then *we* should go ahead and
enable it ourselves in the apr_file_open() code. If that means checking the
mount then we are in for some hassle.
If you want to argue for "violating the principle of least astonishment", again
I'm asserting that corrupted transmission is the greater astonishment :-)
No caller within httpd should be allowed to use this API (just yet) until they
resepect the users choice of disabling sendfile (violating the choice of *not*
using sendfile does much more harm than violating a choice to use it.)
Bill
At 03:29 PM 2/6/2003, Greg Ames wrote:
>Between 2.0.43 and 2.0.44, the performance of SPECweb99 standard dynamic GETs has degraded about 7%. Static files are fine, and ab benchmarks of small dynamic requests are also fine. oprofile + strace shows that the difference with the SPEC dynamic requests is that sendfile is no longer used in .44.
>
>The problem is caused by the APR_SENDFILE_ENABLED flag. If your generator/handler does not explicitly set that flag at apr_file_open() time, the core_output_filter won't use sendfile in .44 or beyond, violating the principle of least astonishment.
>
>The following patch restores API compatibility for non-core modules. It reverses the polarity of the flag so that you get the old behavior unless your module takes explicit action to invoke the new behavior. The EnableSendfile On/Off directive still works as documented, and SPECweb99 performance is back to where it should be.
>
>Greg
>
>
>Index: modules/experimental/mod_mem_cache.c
>===================================================================
>RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_mem_cache.c,v
>retrieving revision 1.92
>diff -u -d -b -r1.92 mod_mem_cache.c
>--- modules/experimental/mod_mem_cache.c 3 Feb 2003 17:53:00 -0000 1.92
>+++ modules/experimental/mod_mem_cache.c 5 Feb 2003 22:36:01 -0000
>@@ -942,7 +942,7 @@
> const char *name;
> /* Open a new XTHREAD handle to the file */
> apr_file_name_get(&name, file);
>- mobj->flags = ((APR_SENDFILE_ENABLED & apr_file_flags_get(file))
>+ mobj->flags = ((APR_SENDFILE_DISABLED & apr_file_flags_get(file))
> | APR_READ | APR_BINARY | APR_XTHREAD | APR_FILE_NOCLEANUP);
> rv = apr_file_open(&tmpfile, name, mobj->flags,
> APR_OS_DEFAULT, r->pool);
>Index: server/core.c
>===================================================================
>RCS file: /home/cvs/httpd-2.0/server/core.c,v
>retrieving revision 1.231
>diff -u -d -b -r1.231 core.c
>--- server/core.c 3 Feb 2003 17:53:18 -0000 1.231
>+++ server/core.c 5 Feb 2003 22:36:01 -0000
>@@ -3319,7 +3319,7 @@
> if ((status = apr_file_open(&fd, r->filename, APR_READ | APR_BINARY
> #if APR_HAS_SENDFILE
> | ((d->enable_sendfile == ENABLE_SENDFILE_OFF)
>- ? 0 : APR_SENDFILE_ENABLED)
>+ ? APR_SENDFILE_DISABLED : 0)
> #endif
> , 0, r->pool)) != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r,
>@@ -3949,7 +3949,7 @@
> }
>
> #if APR_HAS_SENDFILE
>- if (apr_file_flags_get(fd) & APR_SENDFILE_ENABLED) {
>+ if (!(apr_file_flags_get(fd) & APR_SENDFILE_DISABLED)) {
>
> if (c->keepalive == AP_CONN_CLOSE && APR_BUCKET_IS_EOS(last_e)) {
> /* Prepare the socket to be reused */
>Index: srclib/apr/file_io/win32/open.c
>===================================================================
>RCS file: /home/cvs/apr/file_io/win32/open.c,v
>retrieving revision 1.117
>diff -u -d -b -r1.117 open.c
>--- srclib/apr/file_io/win32/open.c 7 Jan 2003 00:52:54 -0000 1.117
>+++ srclib/apr/file_io/win32/open.c 5 Feb 2003 22:36:01 -0000
>@@ -374,7 +374,7 @@
> {
> apr_wchar_t wfname[APR_PATH_MAX];
>
>- if (flag & APR_SENDFILE_ENABLED) {
>+ if (!(flag & APR_SENDFILE_DISABLED)) {
> /* This feature is required to enable sendfile operations
> * against the file on Win32. Also implies APR_XTHREAD.
> */
>@@ -393,11 +393,10 @@
> ELSE_WIN_OS_IS_ANSI {
> handle = CreateFileA(fname, oflags, sharemode,
> NULL, createflags, attributes, 0);
>- if (flag & APR_SENDFILE_ENABLED) {
>- /* This feature is not supported on this platform.
>+ /* sendfile is not supported on this platform.
> */
>- flag &= ~APR_SENDFILE_ENABLED;
>- }
>+ flag |= APR_SENDFILE_DISABLED;
>+
>
> }
> #endif
>Index: srclib/apr/include/apr_file_io.h
>===================================================================
>RCS file: /home/cvs/apr/include/apr_file_io.h,v
>retrieving revision 1.136
>diff -u -d -b -r1.136 apr_file_io.h
>--- srclib/apr/include/apr_file_io.h 24 Jan 2003 19:24:24 -0000 1.136
>+++ srclib/apr/include/apr_file_io.h 5 Feb 2003 22:36:01 -0000
>@@ -103,7 +103,7 @@
> writes across process/machines */
> #define APR_FILE_NOCLEANUP 2048 /**< Do not register a cleanup when the file
> is opened */
>-#define APR_SENDFILE_ENABLED 4096 /**< Advisory flag that this file should
>+#define APR_SENDFILE_DISABLED 4096 /**< Advisory flag that this file does not
> support apr_sendfile operation */
> /** @} */
>