> 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).

>
> Full series updated:
>

[..]

--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel