[PATCH] HTTP/2: fixed sendfile() aio handling

[Sergey Kandaurov]

> On 25 Nov 2021, at 16:54, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> Hello!
> 
> On Thu, Nov 25, 2021 at 04:15:09PM +0300, Sergey Kandaurov wrote:
> 

[ trim ]

>> BTW, what about a similar check in ngx_freebsd_sendfile_chain()
>> used to catch duplicate calls? Both were added in 6422:768e287a6f36.
>> Anyway, it is removed in the sendfile(SF_NODISKIO) rework.
> 
> Yes, thanks, missed this, fixed.  Also fixed wrong return value in 
> the preload handler (it is expected to be ssize_t and match 
> ngx_file_aio_read() result, so using NGX_OK is misleading, replaced 
> with NGX_AGAIN).
> 
> Diff:
> 
> diff --git a/src/http/ngx_http_copy_filter_module.c b/src/http/ngx_http_copy_filter_module.c
> --- a/src/http/ngx_http_copy_filter_module.c
> +++ b/src/http/ngx_http_copy_filter_module.c
> @@ -230,7 +230,7 @@ ngx_http_copy_aio_sendfile_preload(ngx_b
>          * a write event on the main connection
>          */
> 
> -        return NGX_OK;
> +        return NGX_AGAIN;
>     }
> 

Indeed, nice catch.

>     n = ngx_file_aio_read(file->file, buf, 1, file->file_pos, NULL);
> diff --git a/src/os/unix/ngx_freebsd_sendfile_chain.c b/src/os/unix/ngx_freebsd_sendfile_chain.c
> --- a/src/os/unix/ngx_freebsd_sendfile_chain.c
> +++ b/src/os/unix/ngx_freebsd_sendfile_chain.c
> @@ -255,19 +255,6 @@ ngx_freebsd_sendfile_chain(ngx_connectio
> #if (NGX_HAVE_AIO_SENDFILE)
> 
>         if (ebusy) {
> -            if (aio->event.active) {
> -                /*
> -                 * tolerate duplicate calls; they can happen due to subrequests
> -                 * or multiple calls of the next body filter from a filter
> -                 */
> -
> -                if (sent) {
> -                    c->busy_count = 0;
> -                }
> -
> -                return in;
> -            }
> -
>             if (sent == 0) {
>                 c->busy_count++;
> 
> 

With the above adjustment, this part looks good.

> Full patch:
> 
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1637847703 -10800
> #      Thu Nov 25 16:41:43 2021 +0300
> # Node ID c960e182900a8d0b7f3041731ba416f2c7e69d14
> # Parent  3443c02ca1d183fe52bf8af66627c94be2b2f785
> HTTP/2: fixed "task already active" with sendfile in threads.
> 
> With sendfile in threads, "task already active" alerts might appear in logs
> if a write event happens on the main HTTP/2 connection, triggering a sendfile
> in threads while another thread operation is already running.  Observed
> with "aio threads; aio_write on; sendfile on;" and with thread event handlers
> modified to post a write event to the main HTTP/2 connection (though can
> happen without any modifications).
> 
> Similarly, sendfile() with AIO preloading on FreeBSD can trigger duplicate
> aio operation, resulting in "second aio post" alerts.  This is, however,
> harder to reproduce, especially on modern FreeBSD systems, since sendfile()
> usually does not return EBUSY.
> 
> Fix is to avoid starting a sendfile operation if other thread operation
> is active by checking r->aio in the thread handler (and, similarly, in
> aio preload handler).  The added check also makes duplicate calls protection
> redundant, so it is removed.
> 
> [..]

Overall, it looks good.

-- 
Sergey Kandaurov