Very good feedback, we will look into implementing some of them at least.

On Wed, Aug 8, 2018 at 11:16 AM, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> We've thought about offloading more operations to thread pools,
> and that's basically why "aio_write" is a separate directive, so
> more directives like "aio_<some_operation>" can be added in the
> future. It's good to see this is finally happening.
>
> Unfortunately, there are number problems with the patch, and it
> needs more work before it can be committed. In particular:
>
> - The open() calls are offloaded to thread pools unconditionally.
> This may actually reduce performance in various typical
> workloads where open() does not block. Instead, there should be
> an "aio_open on|off" directive, similar to "aio_write".
>
> - The code bypasses open file cache, and uses a direct call
> in the http cache code instead. While it might be ok in your
> setup, it looks like an incomplete solution from the generic point
> of view. A better solution would be to introduce a generic
> interface in ngx_open_cached_file() to allow use of thread
> pools.

Agreed that not everyone wants threaded open() with aio "threads" (if
low CPU overhead is more important than low latency for example). The
semantic of "aio_open on" is uncleared though when aio is on but not
"threads". Having open file cache working with threaded open is nice
to have (although people who need "aio_open" probably also have so
much cached assets that they won't need open file cache).

>
> - The code calls ngx_open_and_stat_file() whithin a thread, which
> is relatively complex function never designed to be thread safe.
> While it looks like currently it is, a better solution would be to
> introduce a separate simple function to execute within a thread,
> similar to ngx_thread_read_handler().

agreed.

>
> - In the ngx_http_file_cache_thread_open() function you allocate
> thread task of size sizeof(ngx_thread_file_open_ctx_t) and store
> it in the file->thread_task. There is no guarantee that this
> task will be compatible with other tasks stored in
> file->thread_task. A better solution would be to extend
> ngx_thread_file_ctx_t and use the same context for all
> file-related threaded operations.

does this matter? the different thread_task won't overlap in their
usage (you can't read a file until after its opened) so there's no
reason they need to be compatible. Isn't that why the task ctx is void
* and ngx_thread_task_alloc takes a generic size?

>
> - A better place for the thread-specific code would be
> src/os/unix/ngx_files.c, where ngx_thread_file_ctx_t and
> existing threaded file operations are defined.

sure

>
> - The code uses memory pool operations wihin a thread,
> specifically ngx_pool_cleanup_add(). Memory pools are not
> thread safe, and the behaviour of this code is therefore
> undefined.

Agreed that it may not be very clean but is this a problem in
practice? The pool is tied to the request and shouldn't shared with
other threads. Asking mostly to clarify my understandings with nginx
memory pools.

>> @@ -278,6 +280,10 @@ ngx_http_file_cache_open(ngx_http_request_t *r)
>> return NGX_AGAIN;
>> }
>>
>> + if (c->opening) {
>> + return ngx_http_file_cache_aio_open(r, c, &rv);
>> + }
>> +
>
> The value of rv is uninitialized here. While it is not strictly a
> bug, but this code indicate bad design. Also, duplicate handling
> of "rv == NGX_DECLINED" below and in the
> ngx_http_file_cache_aio_open() function contributes to the
> problem, as well as passing the rv value to the threaded operation
> where it is not needed. It might be a good idea to re-think the
> design.

Some of this is because we have other internal changes here. Will try
to clean this up better.

The rest of the styling suggestions are all reasonable.

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