> On 14 Dec 2021, at 17:16, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Tue, Nov 30, 2021 at 03:15:50PM +0300, Sergey Kandaurov wrote:
>
>> On Thu, Nov 11, 2021 at 07:21:12AM +0300, Maxim Dounin wrote:
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin@mdounin.ru>
>>> # Date 1636603897 -10800
>>> # Thu Nov 11 07:11:37 2021 +0300
>>> # Node ID 10f96e74ae73e1c53a3fd08e7e1c26754c8969ed
>>> # Parent 98d3beb63f32cbb68d1cdcec385614d32129cad0
>>> Support for sendfile(SF_NOCACHE).
>>>
>>> The SF_NOCACHE flag, introduced in FreeBSD 11 along with the new non-blocking
>>> sendfile() implementation by glebius@, makes it possible to use sendfile()
>>> along with the "directio" directive.
>>>
>>> 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
>>> @@ -256,9 +256,11 @@ ngx_output_chain_as_is(ngx_output_chain_
>>> }
>>> #endif
>>>
>>> +#if !(NGX_HAVE_SENDFILE_NODISKIO)
>>> if (buf->in_file && buf->file->directio) {
>>> return 0;
>>> }
>>> +#endif
>>
>> This probably deserves a comment, why it depends on such a macro test.
>> Though, it should be pretty clear from the commit log.
>
> I also tend to think that the original code is wrong (or, rather,
> too aggressive), and it should only disable sendfile(), much like
> the ctx->sendfile and NGX_SENDFILE_LIMIT checks below.
>
> Changed to the following:
>
> 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
> @@ -256,12 +256,6 @@ ngx_output_chain_as_is(ngx_output_chain_
> }
> #endif
>
> -#if !(NGX_HAVE_SENDFILE_NODISKIO)
> - if (buf->in_file && buf->file->directio) {
> - return 0;
> - }
> -#endif
> -
> sendfile = ctx->sendfile;
>
> #if (NGX_SENDFILE_LIMIT)
> @@ -272,6 +266,19 @@ ngx_output_chain_as_is(ngx_output_chain_
>
> #endif
>
> +#if !(NGX_HAVE_SENDFILE_NODISKIO)
> +
> + /*
> + * With DIRECTIO, disable sendfile() unless sendfile(SF_NOCACHE)
> + * is available.
> + */
> +
> + if (buf->in_file && buf->file->directio) {
> + sendfile = 0;
> + }
> +
> +#endif
> +
> if (!sendfile) {
>
> if (!ngx_buf_in_memory(buf)) {
>
>
> [...]
>

Given the sendfile condition below, I couldn't find such cases with
buffers that have both in_file and ngx_buf_in_memory() tests passed
(at least), such that the patch would have a real behaviour change
resulting in just the in_file bit cleared.
Theoretically, it looks good and more aligned with documentation.

--
Sergey Kandaurov

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