> On 25 Nov 2021, at 16:54, Maxim Dounin <mdounin@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@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

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel