Hello!

On Mon, Dec 27, 2021 at 05:12:39PM +0300, Sergey Kandaurov wrote:

> > On 14 Dec 2021, at 18:28, Maxim Dounin <mdounin@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 <mdounin@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