> On 25 Nov 2021, at 16:54, Maxim Dounin <mdou...@mdounin.ru> wrote: > > Hello! > > On Thu, Nov 25, 2021 at 04:15:09PM +0300, Sergey Kandaurov wrote: >
[ trim ] >> BTW, what about a similar check in ngx_freebsd_sendfile_chain() >> used to catch duplicate calls? Both were added in 6422:768e287a6f36. >> Anyway, it is removed in the sendfile(SF_NODISKIO) rework. > > Yes, thanks, missed this, fixed. Also fixed wrong return value in > the preload handler (it is expected to be ssize_t and match > ngx_file_aio_read() result, so using NGX_OK is misleading, replaced > with NGX_AGAIN). > > Diff: > > 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 > @@ -230,7 +230,7 @@ ngx_http_copy_aio_sendfile_preload(ngx_b > * a write event on the main connection > */ > > - return NGX_OK; > + return NGX_AGAIN; > } > Indeed, nice catch. > n = ngx_file_aio_read(file->file, buf, 1, file->file_pos, NULL); > diff --git a/src/os/unix/ngx_freebsd_sendfile_chain.c > b/src/os/unix/ngx_freebsd_sendfile_chain.c > --- a/src/os/unix/ngx_freebsd_sendfile_chain.c > +++ b/src/os/unix/ngx_freebsd_sendfile_chain.c > @@ -255,19 +255,6 @@ ngx_freebsd_sendfile_chain(ngx_connectio > #if (NGX_HAVE_AIO_SENDFILE) > > if (ebusy) { > - if (aio->event.active) { > - /* > - * tolerate duplicate calls; they can happen due to > subrequests > - * or multiple calls of the next body filter from a filter > - */ > - > - if (sent) { > - c->busy_count = 0; > - } > - > - return in; > - } > - > if (sent == 0) { > c->busy_count++; > > With the above adjustment, this part looks good. > Full patch: > > # HG changeset patch > # User Maxim Dounin <mdou...@mdounin.ru> > # Date 1637847703 -10800 > # Thu Nov 25 16:41:43 2021 +0300 > # Node ID c960e182900a8d0b7f3041731ba416f2c7e69d14 > # Parent 3443c02ca1d183fe52bf8af66627c94be2b2f785 > 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). > > Similarly, sendfile() with AIO preloading on FreeBSD can trigger duplicate > aio operation, resulting in "second aio post" alerts. This is, however, > harder to reproduce, especially on modern FreeBSD systems, since sendfile() > usually does not return EBUSY. > > Fix is to avoid starting a sendfile operation if other thread operation > is active by checking r->aio in the thread handler (and, similarly, in > aio preload handler). The added check also makes duplicate calls protection > redundant, so it is removed. > > [..] Overall, it looks good. -- Sergey Kandaurov _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel