Hello! On Mon, Dec 27, 2021 at 05:12:39PM +0300, Sergey Kandaurov wrote:
> > 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). Sure, thanks for noticing. Fixed and committed. -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
