Hello!

On Thu, Aug 09, 2018 at 02:43:19PM -0700, Ka-Hing Cheung via nginx-devel wrote:

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

As long as the "aio" is set to non-threaded variant so "open" is
not available, it is perfectly fine that "aio_open on"
won't do anything. This is what currently happens with
"aio_write", see http://nginx.org/r/aio_write.

[...]

> > - 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?

While this probably never happens with the current code, original
idea was that file->thread_task can be re-used for other
file-related operations.

[...]

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

In practice the pool can be used in the main thread if some
request-related events happen, or it can be used by another
subrequest.

[...]

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel