Welcome! Log In Create A New Profile

Advanced

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Maxim Dounin
November 22, 2021 10:14PM
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) {

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

[PATCH] HTTP/2: fixed sendfile() aio handling

Maxim Dounin 332 November 10, 2021 10:12PM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Sergey Kandaurov 61 November 16, 2021 07:02AM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Maxim Dounin 57 November 16, 2021 09:42AM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Sergey Kandaurov 162 November 18, 2021 11:48AM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Maxim Dounin 56 November 22, 2021 10:14PM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Sergey Kandaurov 105 November 23, 2021 02:46AM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Maxim Dounin 54 November 23, 2021 10:52PM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Sergey Kandaurov 106 November 24, 2021 10:52AM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Maxim Dounin 113 November 24, 2021 02:08PM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Sergey Kandaurov 136 November 25, 2021 08:16AM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Maxim Dounin 95 November 25, 2021 08:56AM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Sergey Kandaurov 81 November 25, 2021 12:44PM

Re: [PATCH] HTTP/2: fixed sendfile() aio handling

Maxim Dounin 205 November 25, 2021 02:08PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 54
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready