cache: move open to thread pool

[Maxim Dounin]

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 at 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/