apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: [PATCH] Sendfile API compatibility breakage
Date Fri, 07 Feb 2003 00:02:01 GMT
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 */ 
> /** @} */
> 



Mime
View raw message