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

> diff --git a/src/core/ngx_buf.h b/src/core/ngx_buf.h
> --- a/src/core/ngx_buf.h
> +++ b/src/core/ngx_buf.h
> @@ -90,9 +90,6 @@ struct ngx_output_chain_ctx_s {
>
> #if (NGX_HAVE_FILE_AIO || NGX_COMPAT)
> ngx_output_chain_aio_pt aio_handler;
> -#if (NGX_HAVE_AIO_SENDFILE || NGX_COMPAT)
> - ssize_t (*aio_preload)(ngx_buf_t *file);
> -#endif
> #endif
>
> #if (NGX_THREADS || NGX_COMPAT)
> diff --git a/src/core/ngx_connection.h b/src/core/ngx_connection.h
> --- a/src/core/ngx_connection.h
> +++ b/src/core/ngx_connection.h
> @@ -185,7 +185,7 @@ struct ngx_connection_s {
>
> unsigned need_last_buf:1;
>
> -#if (NGX_HAVE_AIO_SENDFILE || NGX_COMPAT)
> +#if (NGX_HAVE_SENDFILE_NODISKIO || NGX_COMPAT)
> unsigned busy_count:2;
> #endif
>
> diff --git a/src/core/ngx_module.h b/src/core/ngx_module.h
> --- a/src/core/ngx_module.h
> +++ b/src/core/ngx_module.h
> @@ -41,7 +41,7 @@
> #define NGX_MODULE_SIGNATURE_3 "0"
> #endif
>
> -#if (NGX_HAVE_AIO_SENDFILE || NGX_COMPAT)
> +#if (NGX_HAVE_SENDFILE_NODISKIO || NGX_COMPAT)
> #define NGX_MODULE_SIGNATURE_4 "1"
> #else
> #define NGX_MODULE_SIGNATURE_4 "0"
> diff --git a/src/core/ngx_output_chain.c b/src/core/ngx_output_chain.c
> --- a/src/core/ngx_output_chain.c
> +++ b/src/core/ngx_output_chain.c
> @@ -29,10 +29,6 @@
>
> static ngx_inline ngx_int_t
> ngx_output_chain_as_is(ngx_output_chain_ctx_t *ctx, ngx_buf_t *buf);
> -#if (NGX_HAVE_AIO_SENDFILE)
> -static ngx_int_t ngx_output_chain_aio_setup(ngx_output_chain_ctx_t *ctx,
> - ngx_file_t *file);
> -#endif
> static ngx_int_t ngx_output_chain_add_copy(ngx_pool_t *pool,
> ngx_chain_t **chain, ngx_chain_t *in);
> static ngx_int_t ngx_output_chain_align_file_buf(ngx_output_chain_ctx_t *ctx,
> @@ -283,12 +279,6 @@ ngx_output_chain_as_is(ngx_output_chain_
> buf->in_file = 0;
> }
>
> -#if (NGX_HAVE_AIO_SENDFILE)
> - if (ctx->aio_preload && buf->in_file) {
> - (void) ngx_output_chain_aio_setup(ctx, buf->file);
> - }
> -#endif
> -
> if (ctx->need_in_memory && !ngx_buf_in_memory(buf)) {
> return 0;
> }
> @@ -301,28 +291,6 @@ ngx_output_chain_as_is(ngx_output_chain_
> }
>
>
> -#if (NGX_HAVE_AIO_SENDFILE)
> -
> -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.

> diff --git a/src/event/ngx_event.h b/src/event/ngx_event.h
> --- a/src/event/ngx_event.h
> +++ b/src/event/ngx_event.h
> @@ -147,10 +147,6 @@ struct ngx_event_aio_s {
>
> ngx_fd_t fd;
>
> -#if (NGX_HAVE_AIO_SENDFILE || NGX_COMPAT)
> - ssize_t (*preload_handler)(ngx_buf_t *file);
> -#endif
> -
> #if (NGX_HAVE_EVENTFD)
> int64_t res;
> #endif
> 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
> @@ -19,10 +19,6 @@ typedef struct {
> static void ngx_http_copy_aio_handler(ngx_output_chain_ctx_t *ctx,
> ngx_file_t *file);
> static void ngx_http_copy_aio_event_handler(ngx_event_t *ev);
> -#if (NGX_HAVE_AIO_SENDFILE)
> -static ssize_t ngx_http_copy_aio_sendfile_preload(ngx_buf_t *file);
> -static void ngx_http_copy_aio_sendfile_event_handler(ngx_event_t *ev);
> -#endif
> #endif
> #if (NGX_THREADS)
> static ngx_int_t ngx_http_copy_thread_handler(ngx_thread_task_t *task,
> @@ -128,9 +124,6 @@ ngx_http_copy_filter(ngx_http_request_t
> #if (NGX_HAVE_FILE_AIO)
> if (ngx_file_aio && clcf->aio == NGX_HTTP_AIO_ON) {
> ctx->aio_handler = ngx_http_copy_aio_handler;
> -#if (NGX_HAVE_AIO_SENDFILE)
> - ctx->aio_preload = ngx_http_copy_aio_sendfile_preload;
> -#endif
> }
> #endif
>
> @@ -207,53 +200,6 @@ ngx_http_copy_aio_event_handler(ngx_even
> ngx_http_run_posted_requests(c);
> }
>
> -
> -#if (NGX_HAVE_AIO_SENDFILE)
> -
> -static ssize_t
> -ngx_http_copy_aio_sendfile_preload(ngx_buf_t *file)
> -{
> - ssize_t n;
> - static u_char buf[1];
> - ngx_event_aio_t *aio;
> - ngx_http_request_t *r;
> - ngx_output_chain_ctx_t *ctx;
> -
> - n = ngx_file_aio_read(file->file, buf, 1, file->file_pos, NULL);
> -
> - if (n == NGX_AGAIN) {
> - aio = file->file->aio;
> - aio->handler = ngx_http_copy_aio_sendfile_event_handler;
> -
> - r = aio->data;
> - r->main->blocked++;
> - r->aio = 1;
> -
> - ctx = ngx_http_get_module_ctx(r, ngx_http_copy_filter_module);
> - ctx->aio = 1;
> - }
> -
> - return n;
> -}
> -
> -
> -static void
> -ngx_http_copy_aio_sendfile_event_handler(ngx_event_t *ev)
> -{
> - ngx_event_aio_t *aio;
> - ngx_http_request_t *r;
> -
> - aio = ev->data;
> - r = aio->data;
> -
> - r->main->blocked--;
> - r->aio = 0;
> - ev->complete = 0;
> -
> - r->connection->write->handler(r->connection->write);
> -}
> -
> -#endif
> #endif
>
>
> 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
> @@ -32,22 +32,21 @@
> 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.

>
> wev = c->write;
> @@ -77,11 +76,6 @@ ngx_freebsd_sendfile_chain(ngx_connectio
> eagain = 0;
> flags = 0;
>
> -#if (NGX_HAVE_AIO_SENDFILE && NGX_SUPPRESS_WARN)
> - aio = NULL;
> - file = NULL;
> -#endif
> -
> header.iovs = headers;
> header.nalloc = NGX_IOVS_PREALLOCATE;
>
> @@ -90,7 +84,7 @@ ngx_freebsd_sendfile_chain(ngx_connectio
>
> for ( ;; ) {
> eintr = 0;
> -#if (NGX_HAVE_AIO_SENDFILE)
> +#if (NGX_HAVE_SENDFILE_NODISKIO)
> ebusy = 0;
> #endif
> prev_send = send;
> @@ -179,9 +173,8 @@ ngx_freebsd_sendfile_chain(ngx_connectio
>
> sent = 0;
>
> -#if (NGX_HAVE_AIO_SENDFILE)
> - aio = file->file->aio;
> - flags = (aio && aio->preload_handler) ? SF_NODISKIO : 0;
> +#if (NGX_HAVE_SENDFILE_NODISKIO)
> + flags = (c->busy_count <= 2) ? SF_NODISKIO : 0;
> #endif
>
> rc = sendfile(file->file->fd, c->fd, file->file_pos,
> @@ -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;
> @@ -252,54 +245,30 @@ 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 (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++;
>
> - if (c->busy_count > 2) {
> - ngx_log_error(NGX_LOG_ALERT, c->log, 0,
> - "sendfile(%V) returned busy again",
> - &file->file->name);
> -
> - c->busy_count = 0;
> - aio->preload_handler = NULL;
> -
> - send = prev_send;
> - continue;
> - }
> + ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0,
> + "sendfile() busy, count:%d", c->busy_count);
>
> } else {
> c->busy_count = 0;
> }
>
> - n = aio->preload_handler(file);
> -
> - if (n > 0) {
> - send = prev_send + sent;
> - continue;
> + if (wev->posted) {
> + ngx_delete_posted_event(wev);
> }
>
> + ngx_post_event(wev, &ngx_posted_next_events);
> +
> + wev->ready = 0;
> return in;
> }
>
> - if (flags == SF_NODISKIO) {
> - c->busy_count = 0;
> - }
> + c->busy_count = 0;
>
> #endif
>
>

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