[PATCH 3 of 4] Upstream: sendfile_max_chunk support

[Maxim Dounin]

Hello!

On Wed, Oct 27, 2021 at 10:54:56PM +0300, Sergey Kandaurov wrote:

> 
> > On 11 Oct 2021, at 21:58, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > 
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1633978615 -10800
> > #      Mon Oct 11 21:56:55 2021 +0300
> > # Node ID c7ef6ce9455b01ee1fdcfd7288c4ac5b3ef0de41
> > # Parent  489323e194e4c3b1a7937c51bd4e1671c70f52f8
> > Upstream: sendfile_max_chunk support.
> > 
> > Previously, connections to upstream servers used sendfile() if it was
> > enabled, but never honored sendfile_max_chunk.  This might result
> > in worker monopolization for a long time if large request bodies
> > are allowed.
> > 
> > diff --git a/src/core/ngx_output_chain.c b/src/core/ngx_output_chain.c
> > --- a/src/core/ngx_output_chain.c
> > +++ b/src/core/ngx_output_chain.c
> > @@ -803,6 +803,10 @@ ngx_chain_writer(void *data, ngx_chain_t
> >         return NGX_ERROR;
> >     }
> > 
> > +    if (chain && c->write->ready) {
> > +        ngx_post_event(c->write, &ngx_posted_next_events);
> > +    }
> > +
> >     for (cl = ctx->out; cl && cl != chain; /* void */) {
> >         ln = cl;
> >         cl = cl->next;
> > 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
> > @@ -1511,8 +1511,9 @@ ngx_http_upstream_check_broken_connectio
> > static void
> > ngx_http_upstream_connect(ngx_http_request_t *r, ngx_http_upstream_t *u)
> > {
> > -    ngx_int_t          rc;
> > -    ngx_connection_t  *c;
> > +    ngx_int_t                  rc;
> > +    ngx_connection_t          *c;
> > +    ngx_http_core_loc_conf_t  *clcf;
> > 
> >     r->connection->log->action = "connecting to upstream";
> > 
> > @@ -1599,10 +1600,12 @@ ngx_http_upstream_connect(ngx_http_reque
> > 
> >     /* init or reinit the ngx_output_chain() and ngx_chain_writer() contexts */
> > 
> > +    clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
> > +
> >     u->writer.out = NULL;
> >     u->writer.last = &u->writer.out;
> >     u->writer.connection = c;
> > -    u->writer.limit = 0;
> > +    u->writer.limit = clcf->sendfile_max_chunk;
> > 
> >     if (u->request_sent) {
> >         if (ngx_http_upstream_reinit(r, u) != NGX_OK) {
> > 
> 
> Just to reiterate, while this has most positive effect with Linux
> sendfile() on fast networks, it doesn't seem to help with typical
> proxying of request body buffered on disk, with sendfile disabled.
> In this case, request body is read in ngx_output_chain_copy_buf()
> iteratively in small client_body_buffer_size parts usually below
> the sendfile_max_chunk limit.  (Yet, this can be limited with
> client_body_buffer_size configured above sendfile_max_chunk.)

Similarly to the identical sendfile_max_chunk code in the write 
filter, this change is not expected to prevent worker 
monopolization in all cases, notably if sendfile is not used and 
the buffer configured is smaller than sendfile_max_chunk.  It can, 
however, prevent worker monopolization with sendfile.

Also, it will naturally prevent hangs on c->send_chain() internal 
limits, such as sendfile()'s 2G limit on Linux (while unlikely, it 
can happen if very large request bodies are allowed), since these 
internal limits are now handled identically to external ones.

> Also, it would barely effect sending body with chunked encoding,
> such as in HTTP/1.1, FastCGI, and with limited frame size in gRPC.

As long as long enough body is readily available on disk, it 
actually will prevent sending more than sendfile_max_chunk in 
total.  With request buffering switched off sending is anyway 
limited with c->read->available on client's connection.

> Anyway, the change looks quite useful and natural to have.

Sure.

-- 
Maxim Dounin
http://mdounin.ru/