> On 23 Nov 2021, at 06:12, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Thu, Nov 18, 2021 at 07:46:48PM +0300, Sergey Kandaurov wrote:
>
>>> On 16 Nov 2021, at 17:41, Maxim Dounin <mdounin@mdounin.ru> wrote:
>>>
>>> On Tue, Nov 16, 2021 at 02:59:46PM +0300, Sergey Kandaurov wrote:
>>>
>>>>> On 11 Nov 2021, at 06:10, Maxim Dounin <mdounin@mdounin.ru> wrote:
>>>>>
>>>>> # HG changeset patch
>>>>> # User Maxim Dounin <mdounin@mdounin.ru>
>>>>> # Date 1636599377 -10800
>>>>> # Thu Nov 11 05:56:17 2021 +0300
>>>>> # Node ID 76e072a6947a221868705c13973de15319c0d921
>>>>> # Parent 82b750b20c5205d685e59031247fe898f011394e
>>>>> HTTP/2: fixed sendfile() aio handling.
>>>>>
>>>>> With sendfile() in threads ("aio threads; sendfile on;"), client connection
>>>>> can block on writing, waiting for sendfile() to complete. In HTTP/2 this
>>>>> might result in the request hang, since an attempt to continue processig
>>>>
>>>> processing
>>>
>>> Fixed, thnx.
>>>
>>>>> in thread event handler will call request's write event handler, which
>>>>> is usually stopped by ngx_http_v2_send_chain(): it does nothing if there
>>>>> are no additional data and stream->queued is set. Further, HTTP/2 resets
>>>>> stream's c->write->ready to 0 if writing blocks, so just fixing
>>>>> ngx_http_v2_send_chain() is not enough.
>>>>>
>>>>> Can be reproduced with test suite on Linux with:
>>>>>
>>>>> TEST_NGINX_GLOBALS_HTTP="aio threads; sendfile on;" prove h2*.t
>>>>>
>>>>> The following tests currently fail: h2_keepalive.t, h2_priority.t,
>>>>> h2_proxy_max_temp_file_size.t, h2.t, h2_trailers.t.
>>>>>
>>>>> Similarly, sendfile() with AIO preloading on FreeBSD can block as well,
>>>>> with similar results. This is, however, harder to reproduce, especially
>>>>> on modern FreeBSD systems, since sendfile() usually do not return EBUSY.
>>>>
>>>> does not
>>>
>>> Fixed, thnx.
>>>
>>>>> Fix is to post a write event on HTTP/2 connection in the thread event
>>>>> handler (and aio preload handler). This ensures that sendfile() will be
>>>>> completed and stream processing will be resumed by HTTP/2 code.
>>>>>
>>>>> 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
>>>>> @@ -250,6 +250,21 @@ ngx_http_copy_aio_sendfile_event_handler
>>>>> r->aio = 0;
>>>>> ev->complete = 0;
>>>>>
>>>>> +#if (NGX_HTTP_V2)
>>>>> +
>>>>> + if (r->stream) {
>>>>> + /*
>>>>> + * for HTTP/2, trigger a write event on the main connection
>>>>> + * to handle sendfile() preload
>>>>> + */
>>>>> +
>>>>> + ngx_post_event(r->stream->connection->connection->write,
>>>>> + &ngx_posted_events);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> +#endif
>>>>> +
>>>>> r->connection->write->handler(r->connection->write);
>>>>> }
>>>>>
>>>>> @@ -323,6 +338,20 @@ ngx_http_copy_thread_event_handler(ngx_e
>>>>> r->main->blocked--;
>>>>> r->aio = 0;
>>>>>
>>>>> +#if (NGX_HTTP_V2)
>>>>> +
>>>>> + if (r->stream) {
>>>>> + /*
>>>>> + * for HTTP/2, trigger a write event on the main connection
>>>>> + * to handle sendfile() in threads
>>>>> + */
>>>>> +
>>>>> + ngx_post_event(r->stream->connection->connection->write,
>>>>> + &ngx_posted_events);
>>>>> + }
>>>>> +
>>>>> +#endif
>>>>> +
>
> [...]
>
> For the record, while testing this patch Sergey found another
> issue with sendfile() in threads and HTTP/2: since HTTP/2 might
> call sendfile() within the main connection, bypassing request
> filter chain, normal r->aio flag checking to prevent multiple
> operations do not work, and this eventually results in "task
> already active" alerts due to duplicate operations being posted.
> With the above patch this issue is much more likely to happen,
> since it intentionally triggers write events on the main HTTP/2
> connection.
>
> Below are two patches: the first one addresses the issue with
> duplicate operations by additionally checking file->thread_task
> before sendfile(), and the second one is a better alternative to
> the above patch which doesn't post additional events on the main
> connection.
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1637635671 -10800
> # Tue Nov 23 05:47:51 2021 +0300
> # Node ID 8a18b0bff1266db221fe35dc08f4483044ea0f86
> # Parent 82b750b20c5205d685e59031247fe898f011394e
> 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).
>
> Fix is to avoid starting a sendfile operation if file->thread_task indicates
> that another thread operation is active.
>
> diff --git a/src/os/unix/ngx_linux_sendfile_chain.c b/src/os/unix/ngx_linux_sendfile_chain.c
> --- a/src/os/unix/ngx_linux_sendfile_chain.c
> +++ b/src/os/unix/ngx_linux_sendfile_chain.c
> @@ -324,6 +324,18 @@ ngx_linux_sendfile_thread(ngx_connection
> "linux sendfile thread: %d, %uz, %O",
> file->file->fd, size, file->file_pos);
>
> + task = file->file->thread_task;
> +
> + if (task && task->event.active) {
> + /*
> + * with HTTP/2, another thread operation might be already running
> + * if sendfile() is called as a result of a write event on the main
> + * connection
> + */
> +
> + return NGX_DONE;
> + }
> +
> task = c->sendfile_task;
>
> if (task == NULL) {
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1637637006 -10800
> # Tue Nov 23 06:10:06 2021 +0300
> # Node ID 45c857a5c64cd99309b0d585f2186d219fa357ed
> # Parent 8a18b0bff1266db221fe35dc08f4483044ea0f86
> HTTP/2: fixed sendfile() aio handling.
>
> With sendfile() in threads ("aio threads; sendfile on;"), client connection
> can block on writing, waiting for sendfile() to complete. In HTTP/2 this
> might result in the request hang, since an attempt to continue processing
> in thread event handler will call request's write event handler, which
> is usually stopped by ngx_http_v2_send_chain(): it does nothing if there
> are no additional data and stream->queued is set. Further, HTTP/2 resets
> stream's c->write->ready to 0 if writing blocks, so just fixing
> ngx_http_v2_send_chain() is not enough.
>
> Can be reproduced with test suite on Linux with:
>
> TEST_NGINX_GLOBALS_HTTP="aio threads; sendfile on;" prove h2*.t
>
> The following tests currently fail: h2_keepalive.t, h2_priority.t,
> h2_proxy_max_temp_file_size.t, h2.t, h2_trailers.t.
>
> Similarly, sendfile() with AIO preloading on FreeBSD can block as well,
> with similar results. This is, however, harder to reproduce, especially
> on modern FreeBSD systems, since sendfile() usually does not return EBUSY.
>
> Fix is to modify ngx_http_v2_send_chain() so it actually tries to send
> data to the main connection when called, and to make sure that
> c->write->ready is set by the relevant event handlers.
>
> 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
> @@ -241,16 +241,32 @@ static void
> ngx_http_copy_aio_sendfile_event_handler(ngx_event_t *ev)
> {
> ngx_event_aio_t *aio;
> + ngx_connection_t *c;
> ngx_http_request_t *r;
>
> aio = ev->data;
> r = aio->data;
> + c = r->connection;
>
> r->main->blocked--;
> r->aio = 0;
> ev->complete = 0;
>
> - r->connection->write->handler(r->connection->write);
> +#if (NGX_HTTP_V2)
> +
> + if (r->stream) {
> + /*
> + * for HTTP/2, update write event to make sure processing will
> + * reach the main connection to handle sendfile() preload
> + */
> +
> + c->write->ready = 1;
> + c->write->active = 0;
> + }
> +
> +#endif
> +
> + c->write->handler(c->write);
> }
>
> #endif
> @@ -323,6 +339,20 @@ ngx_http_copy_thread_event_handler(ngx_e
> r->main->blocked--;
> r->aio = 0;
>
> +#if (NGX_HTTP_V2)
> +
> + if (r->stream) {
> + /*
> + * for HTTP/2, update write event to make sure processing will
> + * reach the main connection to handle sendfile() in threads
> + */
> +
> + c->write->ready = 1;
> + c->write->active = 0;
> + }
> +
> +#endif
> +
> if (r->done) {
> /*
> * trigger connection event handler if the subrequest was
> diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c
> +++ b/src/http/ngx_http_upstream.c
> @@ -3905,6 +3905,20 @@ ngx_http_upstream_thread_event_handler(n
> r->main->blocked--;
> r->aio = 0;
>
> +#if (NGX_HTTP_V2)
> +
> + if (r->stream) {
> + /*
> + * for HTTP/2, update write event to make sure processing will
> + * reach the main connection to handle sendfile() in threads
> + */
> +
> + c->write->ready = 1;
> + c->write->active = 0;
> + }
> +
> +#endif
> +
> if (r->done) {
> /*
> * trigger connection event handler if the subrequest was
> diff --git a/src/http/v2/ngx_http_v2_filter_module.c b/src/http/v2/ngx_http_v2_filter_module.c
> --- a/src/http/v2/ngx_http_v2_filter_module.c
> +++ b/src/http/v2/ngx_http_v2_filter_module.c
> @@ -1432,6 +1432,9 @@ ngx_http_v2_send_chain(ngx_connection_t
> size = 0;
> #endif
>
> + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, fc->log, 0,
> + "http2 send chain: %p", in);
> +
> while (in) {
> size = ngx_buf_size(in->buf);
>
> @@ -1450,12 +1453,8 @@ ngx_http_v2_send_chain(ngx_connection_t
> return NGX_CHAIN_ERROR;
> }
>
> - if (stream->queued) {
> - fc->write->active = 1;
> - fc->write->ready = 0;
> -
> - } else {
> - fc->buffered &= ~NGX_HTTP_V2_BUFFERED;
> + if (ngx_http_v2_filter_send(fc, stream) == NGX_ERROR) {
> + return NGX_CHAIN_ERROR;
> }
>
> return NULL;
> @@ -1464,9 +1463,16 @@ ngx_http_v2_send_chain(ngx_connection_t
> h2c = stream->connection;
>
> if (size && ngx_http_v2_flow_control(h2c, stream) == NGX_DECLINED) {
> - fc->write->active = 1;
> - fc->write->ready = 0;
> - return in;
> +
> + if (ngx_http_v2_filter_send(fc, stream) == NGX_ERROR) {
> + return NGX_CHAIN_ERROR;
> + }
> +
> + if (ngx_http_v2_flow_control(h2c, stream) == NGX_DECLINED) {
> + fc->write->active = 1;
> + fc->write->ready = 0;
> + return in;
> + }
> }
>
> if (in->buf->tag == (ngx_buf_tag_t) &ngx_http_v2_filter_get_shadow) {
> @@ -1809,6 +1815,11 @@ ngx_http_v2_waiting_queue(ngx_http_v2_co
> static ngx_inline ngx_int_t
> ngx_http_v2_filter_send(ngx_connection_t *fc, ngx_http_v2_stream_t *stream)
> {
> + if (stream->queued == 0) {
> + fc->buffered &= ~NGX_HTTP_V2_BUFFERED;
> + return NGX_OK;
> + }
> +
> stream->blocked = 1;
>
> if (ngx_http_v2_send_output_queue(stream->connection) == NGX_ERROR) {
>

Looks fine.

JFTR: all known issues should be fixed now.

--
Sergey Kandaurov

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