> 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

> 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

>
> 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, currently it looks as below.

2021/11/16 11:30:21 [debug] 3704204#3704206: run task #1 in thread pool "default"
2021/11/16 11:30:21 [debug] 3704204#3704206: linux sendfile thread handler
...
2021/11/16 11:30:21 [debug] 3704204#3704204: run completion handler for task #1
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 http thread: "/1?" done:0
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 post event 00005613DD6E9A90
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 http upstream process downstream
....
2021/11/16 11:30:21 [debug] 3704204#3704204: posted event 00005613DD6E9A90
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 delete posted event 00005613DD6E9A90
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 http2 write handler
....
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 http2:1 DATA frame 00005613DD596C88 was sent
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 post event 00005613DD58D9A0
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 http2 frame sent: 00005613DD596C88 sid:1 bl:1 len:8192
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 update posted event 00005613DD58D9A0
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 http2:1 DATA frame 00005613DD597750 was sent partially
2021/11/16 11:30:21 [debug] 3704204#3704204: posted event 00005613DD58D9A0
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 delete posted event 00005613DD58D9A0
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 http run request: "/1?"
2021/11/16 11:30:21 [debug] 3704204#3704204: *1 http upstream process downstream

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


> 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().

--
Sergey Kandaurov

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