> On 16 Nov 2021, at 17:41, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> 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
>>> +
>>
>> This thread event handler is used not only for sendfile() completion,
>> but also to complete reading in threads a buffered upstream response.
>> In this case, posting a write event on HTTP/2 connection looks
>> unnecessary, since there is no sendfile() in action, it will do nothing.
>> On the other hand, if it is indeed used to complete a sendfile() task,
>> which needs to invoke http2 write handler, calling write_event_handler()
>> directly from thread event handler seems to be redundant: it could be
>> optimized away since http2 write handler will normally end up in posting
>> a write event on the main connection, anyway, see the call sequence
>> ngx_http_v2_write_handler() -> ngx_http_v2_send_output_queue()
>> -> ngx_http_v2_data_frame_handler() -> ngx_http_v2_handle_stream().
>
> [...]
>
>> So, it could be narrowed down, something like the aio preload handler:
>>
>> diff -r 76e072a6947a -r 5f48b9a797d1 src/http/ngx_http_copy_filter_module.c
>> --- a/src/http/ngx_http_copy_filter_module.c Thu Nov 11 05:56:17 2021 +0300
>> +++ b/src/http/ngx_http_copy_filter_module.c Mon Nov 15 21:04:26 2021 +0000
>> @@ -340,7 +340,7 @@
>>
>> #if (NGX_HTTP_V2)
>>
>> - if (r->stream) {
>> + if (r->stream && r->stream->connection->connection->sendfile_task) {
>> /*
>> * for HTTP/2, trigger a write event on the main connection
>> * to handle sendfile() in threads
>> @@ -348,6 +348,7 @@
>>
>> ngx_post_event(r->stream->connection->connection->write,
>> &ngx_posted_events);
>> + return;
>> }
>>
>> #endif
>
> This "return" won't work, since even with sendfile() enabled and
> being used, the handler can be called for non-sendfile operations
> as well.
>
> That is, both posting an event to the main connection _and_
> calling request write handler are required. This might be
> redundant in some cases, but there is no reasonable way to avoid
> this with sendfile() enabled.
>
> Checking sendfile_task might be used to avoid extra posted event
> with sendfile disabled, but it looks overcomplicated to me and I
> don't think it worth the effort. It's at most a minor
> optimization.
>

Fair enough.

>>> 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, 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
>>> +
>>> if (r->done) {
>>> /*
>>> * trigger connection event handler if the subrequest was
>>>
>>
>> I could not figure out, how this part is related, since upstream
>> thread handler is only enabled with "aio_write on;" to write down
>> a buffered upstream response to disk. It doesn't seem to be used
>> with sendfile().
>
> Thread handlers are set on per-file basis. As a result, if
> aio_write is enabled, the ngx_http_upstream_thread_event_handler()
> handler can be used for sendfile() as well.

Indeed, it is the case. I was able to reproduce it with a variant
of slice.t, updated to "listen .. http2". The patch fixes it.

>
> Also note the following "trigger connection event handler..."
> part: it is also only needed for sendfile(), yet present in the
> ngx_http_upstream_thread_event_handler().

Yep, I've triggered the original problem fixed with the existing condition,
though I had to back out sendfile_max_chunk rework with posted next events
to catch it, as otherwise the write event was observed to be always posted.

--
Sergey Kandaurov

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