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.

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

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

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel