cache: move open to thread pool

[Ka-Hing Cheung]

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

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