> On 14 Dec 2021, at 18:28, Maxim Dounin <mdou...@mdounin.ru> wrote: > > Hello! > > On Tue, Dec 14, 2021 at 05:15:47PM +0300, Maxim Dounin wrote: > >> On Tue, Nov 30, 2021 at 03:05:03PM +0300, Sergey Kandaurov wrote: >> >>> On Thu, Nov 11, 2021 at 07:21:10AM +0300, Maxim Dounin wrote: >>>> # HG changeset patch >>>> # User Maxim Dounin <mdou...@mdounin.ru> >>>> # Date 1636603886 -10800 >>>> # Thu Nov 11 07:11:26 2021 +0300 >>>> # Node ID 4a954e89b1ae8539bbe08c5afc1d5c9828d82d6f >>>> # Parent 0fb75ef9dbca698e5e855145cf6a12180a36d400 >>>> Simplified sendfile(SF_NODISKIO) usage. >>>> >>>> Starting with FreeBSD 11, there is no need to use AIO operations to preload >>>> data into cache for sendfile(SF_NODISKIO) to work. Instead, sendfile() >>>> handles non-blocking loading data from disk by itself. It still can, >>>> however, >>>> return EBUSY if a page is already being loaded (for example, by a different >>>> process). If this happens, we now post an event for the next event loop >>>> iteration, so sendfile() is retried "after a short period", as manpage >>>> recommends. >>>> >>>> The limit of the number of EBUSY tolerated without any progress is >>>> preserved, >>>> but now it does not result in an alert, since on an idle system event loop >>>> iteration might be very short and EBUSY can happen many times in a row. >>>> Instead, SF_NODISKIO is simply disabled for one call once the limit is >>>> reached. >>>> >>>> With this change, sendfile(SF_NODISKIO) is now used automatically as long >>>> as >>>> sendfile() is enabled, and no longer requires "aio on;". >>>> >>>> diff --git a/auto/os/freebsd b/auto/os/freebsd >>>> --- a/auto/os/freebsd >>>> +++ b/auto/os/freebsd >>>> @@ -44,12 +44,10 @@ if [ $osreldate -gt 300007 ]; then >>>> CORE_SRCS="$CORE_SRCS $FREEBSD_SENDFILE_SRCS" >>>> fi >>>> >>>> -if [ $NGX_FILE_AIO = YES ]; then >>>> - if [ $osreldate -gt 502103 ]; then >>>> - echo " + sendfile()'s SF_NODISKIO found" >>>> +if [ $osreldate -gt 1100000 ]; then >>>> + echo " + sendfile()'s SF_NODISKIO found" >>>> >>>> - have=NGX_HAVE_AIO_SENDFILE . auto/have >>>> - fi >>>> + have=NGX_HAVE_SENDFILE_NODISKIO . auto/have >>>> fi >>>> >>>> # POSIX semaphores >>> >>> We could check the exact __FreeBSD_version number 1100093 that was >>> at the time the new sendfile() appeared, which is more accurate. >>> >>> https://cgit.freebsd.org/src/commit/?id=2bab0c553588 >>> https://cgit.freebsd.org/src/tree/sys/sys/param.h?id=2bab0c553588#n48 >>> >>> Unfortunately, it was not bumped (same as with SF_NODISKIO in 5.2.1). >> >> Yes, probably. But the next version bump would be more >> appropriate, changed to 1100093. Similarly to 502103, which is >> the next version after SF_NODISKIO introduction: >> >> https://cgit.freebsd.org/src/commit/?id=b49d824e8bc1 >> https://cgit.freebsd.org/src/tree/sys/sys/param.h?id=b49d824e8bc1 >>
I'm fine with it. >> [...] >> >>>> -static ngx_int_t >>>> -ngx_output_chain_aio_setup(ngx_output_chain_ctx_t *ctx, ngx_file_t *file) >>>> -{ >>>> - ngx_event_aio_t *aio; >>>> - >>>> - if (file->aio == NULL && ngx_file_aio_init(file, ctx->pool) != >>>> NGX_OK) { >>>> - return NGX_ERROR; >>>> - } >>>> - >>>> - aio = file->aio; >>>> - >>>> - aio->data = ctx->filter_ctx; >>>> - aio->preload_handler = ctx->aio_preload; >>>> - >>>> - return NGX_OK; >>>> -} >>>> - >>>> -#endif >>>> - >>>> - >>>> static ngx_int_t >>>> ngx_output_chain_add_copy(ngx_pool_t *pool, ngx_chain_t **chain, >>>> ngx_chain_t *in) >>> >>> After this change, ngx_file_aio_init() doesn't need to be external. >>> It is only used in ngx_file_aio_read() in corresponding ngx_*_aio_read.c. >> >> In practice, yes. In theory, it might be needed if we'll ever >> implement other AIO operations, such as aio_write(), and/or will >> reconsider thread-based interface to use the aio structure. So I >> would rather preserve it as is, at least for now. >> Ok, I won't insist. >> [...] >> >>>> ngx_chain_t * >>>> ngx_freebsd_sendfile_chain(ngx_connection_t *c, ngx_chain_t *in, off_t >>>> limit) >>>> { >>>> - int rc, flags; >>>> - off_t send, prev_send, sent; >>>> - size_t file_size; >>>> - ssize_t n; >>>> - ngx_uint_t eintr, eagain; >>>> - ngx_err_t err; >>>> - ngx_buf_t *file; >>>> - ngx_event_t *wev; >>>> - ngx_chain_t *cl; >>>> - ngx_iovec_t header, trailer; >>>> - struct sf_hdtr hdtr; >>>> - struct iovec headers[NGX_IOVS_PREALLOCATE]; >>>> - struct iovec trailers[NGX_IOVS_PREALLOCATE]; >>>> -#if (NGX_HAVE_AIO_SENDFILE) >>>> - ngx_uint_t ebusy; >>>> - ngx_event_aio_t *aio; >>>> + int rc, flags; >>>> + off_t send, prev_send, sent; >>>> + size_t file_size; >>>> + ssize_t n; >>>> + ngx_uint_t eintr, eagain; >>>> + ngx_err_t err; >>>> + ngx_buf_t *file; >>>> + ngx_event_t *wev; >>>> + ngx_chain_t *cl; >>>> + ngx_iovec_t header, trailer; >>>> + struct sf_hdtr hdtr; >>>> + struct iovec headers[NGX_IOVS_PREALLOCATE]; >>>> + struct iovec trailers[NGX_IOVS_PREALLOCATE]; >>>> +#if (NGX_HAVE_SENDFILE_NODISKIO) >>>> + ngx_uint_t ebusy; >>>> #endif >>> >>> After ngx_event_aio_t *aio variable removal, >>> this block could be placed under "eintr, eagain" line >>> (which by itself looks unsorted). >>> >>> The remaining part looks good to me. >> >> Yes, thanks. Changed to: >> >> 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 >> @@ -36,18 +36,18 @@ ngx_freebsd_sendfile_chain(ngx_connectio >> off_t send, prev_send, sent; >> size_t file_size; >> ssize_t n; >> - ngx_uint_t eintr, eagain; >> ngx_err_t err; >> ngx_buf_t *file; >> + ngx_uint_t eintr, eagain; >> +#if (NGX_HAVE_SENDFILE_NODISKIO) >> + ngx_uint_t ebusy; >> +#endif >> ngx_event_t *wev; >> ngx_chain_t *cl; >> ngx_iovec_t header, trailer; >> struct sf_hdtr hdtr; >> struct iovec headers[NGX_IOVS_PREALLOCATE]; >> struct iovec trailers[NGX_IOVS_PREALLOCATE]; >> -#if (NGX_HAVE_SENDFILE_NODISKIO) >> - ngx_uint_t ebusy; >> -#endif >> >> wev = c->write; >> >> Looks good to me. >> Full patch, updated: > > [...] > >> 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 > > [...] > >> @@ -199,7 +192,7 @@ ngx_freebsd_sendfile_chain(ngx_connectio >> eintr = 1; >> break; >> >> -#if (NGX_HAVE_AIO_SENDFILE) >> +#if (NGX_HAVE_SENDFILE_NODISKIO) >> case NGX_EBUSY: >> ebusy = 1; >> break; >> @@ -258,35 +251,24 @@ ngx_freebsd_sendfile_chain(ngx_connectio >> if (sent == 0) { >> c->busy_count++; >> > > Err, mismerge here. Should be: > > 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 > @@ -245,7 +245,7 @@ ngx_freebsd_sendfile_chain(ngx_connectio > > in = ngx_chain_update_sent(in, sent); > > -#if (NGX_HAVE_AIO_SENDFILE) > +#if (NGX_HAVE_SENDFILE_NODISKIO) > > if (ebusy) { > if (sent == 0) { > Looks like this hunk appears in the updated patch 3, should belong to patch 2 (SF_NODISKIO refactoring). > > Full series updated: > [..] -- Sergey Kandaurov _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel