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