Thanks for all the feedback! This is an updated version of the patch.
There may still be some coding style issues (which will be addressed),
but should address most of the comments other than these two:

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

The complications with this just didn't seem worth it. aio_open makes
the most impact if a large number of distinct files need to be opened
and that's when open_file_cache is last effectiv. Instead I made
aio_open to only take effect if open_file_cache is off.

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

I kept using ngx_open_and_stat_file() as we are looking into moving
other types of open into thread pool, so we will probably need to have
similar logic anyway.

commit 3c67f1d972404bda7713839186a91cb18dbc96be
Author: Ka-Hing Cheung <kahing@cloudflare.com>
Date: Fri Aug 3 13:37:58 2018 -0700

move open to thread pool

At cloudflare we found that open() can block a long time, especially
at p99 and p999. Moving it to thread pool improved overall p99 by 6x
or so during peak time.

This introduces an aio_open option. When set to 'on' the open will be
done in a thread pool. open_file_cache has to be disabled for aio_open
to take effect.

thread pool does increase CPU usage somewhat but we have not measured
difference in CPU usage for stock "aio threads" compared to after this
patch.

Only the cache hit open() is moved to thread pool.

diff --git src/core/ngx_open_file_cache.c src/core/ngx_open_file_cache.c
index b23ee78d..9177ebfd 100644
--- src/core/ngx_open_file_cache.c
+++ src/core/ngx_open_file_cache.c
@@ -35,8 +35,6 @@ static ngx_fd_t ngx_open_file_wrapper(ngx_str_t *name,
ngx_int_t access, ngx_log_t *log);
static ngx_int_t ngx_file_info_wrapper(ngx_str_t *name,
ngx_open_file_info_t *of, ngx_file_info_t *fi, ngx_log_t *log);
-static ngx_int_t ngx_open_and_stat_file(ngx_str_t *name,
- ngx_open_file_info_t *of, ngx_log_t *log);
static void ngx_open_file_add_event(ngx_open_file_cache_t *cache,
ngx_cached_open_file_t *file, ngx_open_file_info_t *of, ngx_log_t *log);
static void ngx_open_file_cleanup(void *data);
@@ -836,7 +834,7 @@ ngx_file_info_wrapper(ngx_str_t *name,
ngx_open_file_info_t *of,
}


-static ngx_int_t
+ngx_int_t
ngx_open_and_stat_file(ngx_str_t *name, ngx_open_file_info_t *of,
ngx_log_t *log)
{
diff --git src/core/ngx_open_file_cache.h src/core/ngx_open_file_cache.h
index d119c129..144744c0 100644
--- src/core/ngx_open_file_cache.h
+++ src/core/ngx_open_file_cache.h
@@ -7,6 +7,7 @@

#include <ngx_config.h>
#include <ngx_core.h>
+#include <ngx_queue.h>


#ifndef _NGX_OPEN_FILE_CACHE_H_INCLUDED_
@@ -124,6 +125,8 @@ ngx_open_file_cache_t
*ngx_open_file_cache_init(ngx_pool_t *pool,
ngx_uint_t max, time_t inactive);
ngx_int_t ngx_open_cached_file(ngx_open_file_cache_t *cache, ngx_str_t *name,
ngx_open_file_info_t *of, ngx_pool_t *pool);
+ngx_int_t ngx_open_and_stat_file(ngx_str_t *name,
+ ngx_open_file_info_t *of, ngx_log_t *log);


#endif /* _NGX_OPEN_FILE_CACHE_H_INCLUDED_ */
diff --git src/http/ngx_http_cache.h src/http/ngx_http_cache.h
index f9e96640..8940405a 100644
--- src/http/ngx_http_cache.h
+++ src/http/ngx_http_cache.h
@@ -97,6 +97,7 @@ struct ngx_http_cache_s {

#if (NGX_THREADS || NGX_COMPAT)
ngx_thread_task_t *thread_task;
+ ngx_int_t open_rv;
#endif

ngx_msec_t lock_timeout;
@@ -114,6 +115,9 @@ struct ngx_http_cache_s {
unsigned exists:1;
unsigned temp_file:1;
unsigned purged:1;
+#if (NGX_THREADS || NGX_COMPAT)
+ unsigned opening:1;
+#endif
unsigned reading:1;
unsigned secondary:1;
unsigned background:1;
diff --git src/http/ngx_http_core_module.c src/http/ngx_http_core_module.c
index c57ec00c..1c7b26c2 100644
--- src/http/ngx_http_core_module.c
+++ src/http/ngx_http_core_module.c
@@ -420,6 +420,13 @@ static ngx_command_t ngx_http_core_commands[] = {
offsetof(ngx_http_core_loc_conf_t, aio_write),
NULL },

+ { ngx_string("aio_open"),
+ NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_FLAG,
+ ngx_conf_set_flag_slot,
+ NGX_HTTP_LOC_CONF_OFFSET,
+ offsetof(ngx_http_core_loc_conf_t, aio_open),
+ NULL },
+
{ ngx_string("read_ahead"),
NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
ngx_conf_set_size_slot,
@@ -3380,6 +3387,7 @@ ngx_http_core_create_loc_conf(ngx_conf_t *cf)
clcf->subrequest_output_buffer_size = NGX_CONF_UNSET_SIZE;
clcf->aio = NGX_CONF_UNSET;
clcf->aio_write = NGX_CONF_UNSET;
+ clcf->aio_open = NGX_CONF_UNSET;
#if (NGX_THREADS)
clcf->thread_pool = NGX_CONF_UNSET_PTR;
clcf->thread_pool_value = NGX_CONF_UNSET_PTR;
@@ -3605,6 +3613,7 @@ ngx_http_core_merge_loc_conf(ngx_conf_t *cf,
void *parent, void *child)
(size_t) ngx_pagesize);
ngx_conf_merge_value(conf->aio, prev->aio, NGX_HTTP_AIO_OFF);
ngx_conf_merge_value(conf->aio_write, prev->aio_write, 0);
+ ngx_conf_merge_value(conf->aio_open, prev->aio_open, 0);
#if (NGX_THREADS)
ngx_conf_merge_ptr_value(conf->thread_pool, prev->thread_pool, NULL);
ngx_conf_merge_ptr_value(conf->thread_pool_value, prev->thread_pool_value,
diff --git src/http/ngx_http_core_module.h src/http/ngx_http_core_module.h
index 4c6da7c0..8498eb63 100644
--- src/http/ngx_http_core_module.h
+++ src/http/ngx_http_core_module.h
@@ -382,6 +382,7 @@ struct ngx_http_core_loc_conf_s {
ngx_flag_t sendfile; /* sendfile */
ngx_flag_t aio; /* aio */
ngx_flag_t aio_write; /* aio_write */
+ ngx_flag_t aio_open; /* aio_open */
ngx_flag_t tcp_nopush; /* tcp_nopush */
ngx_flag_t tcp_nodelay; /* tcp_nodelay */
ngx_flag_t reset_timedout_connection; /* reset_timedout_connection */
diff --git src/http/ngx_http_file_cache.c src/http/ngx_http_file_cache.c
index 56866fa4..10a29d93 100644
--- src/http/ngx_http_file_cache.c
+++ src/http/ngx_http_file_cache.c
@@ -18,6 +18,8 @@ static void
ngx_http_file_cache_lock_wait(ngx_http_request_t *r,
ngx_http_cache_t *c);
static ngx_int_t ngx_http_file_cache_read(ngx_http_request_t *r,
ngx_http_cache_t *c);
+static ngx_int_t ngx_http_file_cache_aio_open(ngx_http_request_t *r,
+ ngx_http_cache_t *c, ngx_int_t rv);
static ssize_t ngx_http_file_cache_aio_read(ngx_http_request_t *r,
ngx_http_cache_t *c);
#if (NGX_HAVE_FILE_AIO)
@@ -268,9 +270,7 @@ ngx_http_file_cache_open(ngx_http_request_t *r)
ngx_uint_t test;
ngx_http_cache_t *c;
ngx_pool_cleanup_t *cln;
- ngx_open_file_info_t of;
ngx_http_file_cache_t *cache;
- ngx_http_core_loc_conf_t *clcf;

c = r->cache;

@@ -278,6 +278,12 @@ ngx_http_file_cache_open(ngx_http_request_t *r)
return NGX_AGAIN;
}

+#if (NGX_THREADS)
+ if (c->opening) {
+ return ngx_http_file_cache_aio_open(r, c, c->open_rv);
+ }
+#endif
+
if (c->reading) {
return ngx_http_file_cache_read(r, c);
}
@@ -339,58 +345,10 @@ ngx_http_file_cache_open(ngx_http_request_t *r)
return NGX_ERROR;
}

- if (!test) {
- goto done;
- }
-
- clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
-
- ngx_memzero(&of, sizeof(ngx_open_file_info_t));
-
- of.uniq = c->uniq;
- of.valid = clcf->open_file_cache_valid;
- of.min_uses = clcf->open_file_cache_min_uses;
- of.events = clcf->open_file_cache_events;
- of.directio = NGX_OPEN_FILE_DIRECTIO_OFF;
- of.read_ahead = clcf->read_ahead;
-
- if (ngx_open_cached_file(clcf->open_file_cache, &c->file.name,
&of, r->pool)
- != NGX_OK)
- {
- switch (of.err) {
-
- case 0:
- return NGX_ERROR;
-
- case NGX_ENOENT:
- case NGX_ENOTDIR:
- goto done;
-
- default:
- ngx_log_error(NGX_LOG_CRIT, r->connection->log, of.err,
- ngx_open_file_n " \"%s\" failed", c->file.name.data);
- return NGX_ERROR;
- }
+ if (test) {
+ return ngx_http_file_cache_aio_open(r, c, rv);
}

- ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
- "http file cache fd: %d", of.fd);
-
- c->file.fd = of.fd;
- c->file.log = r->connection->log;
- c->uniq = of.uniq;
- c->length = of.size;
- c->fs_size = (of.fs_size + cache->bsize - 1) / cache->bsize;
-
- c->buf = ngx_create_temp_buf(r->pool, c->body_start);
- if (c->buf == NULL) {
- return NGX_ERROR;
- }
-
- return ngx_http_file_cache_read(r, c);
-
-done:
-
if (rv == NGX_DECLINED) {
return ngx_http_file_cache_lock(r, c);
}
@@ -398,7 +356,6 @@ done:
return rv;
}

-
static ngx_int_t
ngx_http_file_cache_lock(ngx_http_request_t *r, ngx_http_cache_t *c)
{
@@ -663,6 +620,106 @@ ngx_http_file_cache_read(ngx_http_request_t *r,
ngx_http_cache_t *c)
}


+static ngx_int_t
+ngx_http_file_cache_aio_open(ngx_http_request_t *r, ngx_http_cache_t *c,
+ ngx_int_t rv)
+{
+ ngx_int_t rc;
+ ngx_open_file_info_t of = { 0 };
+ ngx_http_file_cache_t *cache;
+ ngx_http_core_loc_conf_t *clcf;
+
+ clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
+
+ of.uniq = c->uniq;
+ of.valid = clcf->open_file_cache_valid;
+ of.min_uses = clcf->open_file_cache_min_uses;
+ of.events = clcf->open_file_cache_events;
+ of.directio = NGX_OPEN_FILE_DIRECTIO_OFF;
+ of.read_ahead = clcf->read_ahead;
+
+#if (NGX_THREADS)
+ if (clcf->aio == NGX_HTTP_AIO_THREADS && clcf->aio_open &&
+ clcf->open_file_cache == NULL) {
+
+ c->file.thread_task = c->thread_task;
+ c->file.thread_handler = ngx_http_cache_thread_handler;
+ c->file.thread_ctx = r;
+ c->open_rv = rv;
+
+ rc = ngx_thread_open(&c->file, &of, r->pool);
+
+ c->opening = (rc == NGX_AGAIN);
+ c->thread_task = c->file.thread_task;
+
+ if (rc == NGX_AGAIN) {
+ return rc;
+ } else if (of.fd != NGX_INVALID_FILE) {
+ ngx_pool_cleanup_t *cln;
+ ngx_pool_cleanup_file_t *clnf;
+
+ cln = ngx_pool_cleanup_add(r->pool,
sizeof(ngx_pool_cleanup_file_t));
+ if (cln == NULL) {
+ ngx_close_file(of.fd);
+ goto done;
+ }
+
+ cln->handler = ngx_pool_cleanup_file;
+ clnf = cln->data;
+
+ clnf->fd = of.fd;
+ clnf->name = r->cache->file.name.data;
+ clnf->log = r->connection->log;
+ goto post_open;
+ }
+ }
+#endif
+
+ rc = ngx_open_cached_file(clcf->open_file_cache, &c->file.name,
&of, r->pool);
+
+#if (NGX_THREADS)
+post_open:
+#endif
+
+ if (rc != NGX_OK) {
+ switch (of.err) {
+ case NGX_OK:
+ return NGX_ERROR;
+ case NGX_ENOENT:
+ case NGX_ENOTDIR:
+ goto done;
+
+ default:
+ ngx_log_error(NGX_LOG_CRIT, r->connection->log, of.err,
+ ngx_open_file_n " \"%s\" failed", c->file.name.data);
+ return NGX_ERROR;
+ }
+ }
+
+ ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+ "http file cache fd: %d", of.fd);
+
+ cache = c->file_cache;
+ c->file.fd = of.fd;
+ c->file.log = r->connection->log;
+ c->uniq = of.uniq;
+ c->length = of.size;
+ c->fs_size = (of.fs_size + cache->bsize - 1) / cache->bsize;
+
+ c->buf = ngx_create_temp_buf(r->pool, c->body_start);
+ if (c->buf == NULL) {
+ return NGX_ERROR;
+ }
+
+ return ngx_http_file_cache_read(r, c);
+
+done:
+ if (rv == NGX_DECLINED) {
+ return ngx_http_file_cache_lock(r, c);
+ }
+ return rv;
+}
+
static ssize_t
ngx_http_file_cache_aio_read(ngx_http_request_t *r, ngx_http_cache_t *c)
{
@@ -751,30 +808,14 @@ ngx_http_cache_aio_event_handler(ngx_event_t *ev)
static ngx_int_t
ngx_http_cache_thread_handler(ngx_thread_task_t *task, ngx_file_t *file)
{
- ngx_str_t name;
ngx_thread_pool_t *tp;
ngx_http_request_t *r;
- ngx_http_core_loc_conf_t *clcf;

r = file->thread_ctx;

- clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
- tp = clcf->thread_pool;
-
+ tp = ngx_http_request_get_thread_pool(r);
if (tp == NULL) {
- if (ngx_http_complex_value(r, clcf->thread_pool_value, &name)
- != NGX_OK)
- {
- return NGX_ERROR;
- }
-
- tp = ngx_thread_pool_get((ngx_cycle_t *) ngx_cycle, &name);
-
- if (tp == NULL) {
- ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
- "thread pool \"%V\" not found", &name);
- return NGX_ERROR;
- }
+ return NGX_ERROR;
}

task->event.data = r;
diff --git src/http/ngx_http_request.c src/http/ngx_http_request.c
index 97900915..228cda3d 100644
--- src/http/ngx_http_request.c
+++ src/http/ngx_http_request.c
@@ -3700,3 +3700,39 @@ ngx_http_log_error_handler(ngx_http_request_t
*r, ngx_http_request_t *sr,

return buf;
}
+
+
+#if (NGX_THREADS)
+
+ngx_thread_pool_t *
+ngx_http_request_get_thread_pool(ngx_http_request_t *r)
+{
+ ngx_str_t name;
+ ngx_thread_pool_t *tp;
+ ngx_http_core_loc_conf_t *clcf;
+
+ clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
+ tp = clcf->thread_pool;
+
+ if (tp == NULL) {
+ if (ngx_http_complex_value(r, clcf->thread_pool_value, &name)
+ != NGX_OK)
+ {
+ return NULL;
+ }
+
+ tp = ngx_thread_pool_get((ngx_cycle_t *) ngx_cycle, &name);
+
+ if (tp == NULL) {
+ ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
+ "thread pool \"%V\" not found", &name);
+ return NULL;
+ }
+
+ clcf->thread_pool = tp;
+ }
+
+ return tp;
+}
+
+#endif
diff --git src/http/ngx_http_request.h src/http/ngx_http_request.h
index 6bfff96e..12df9c76 100644
--- src/http/ngx_http_request.h
+++ src/http/ngx_http_request.h
@@ -605,4 +605,10 @@ extern ngx_http_header_out_t ngx_http_headers_out[];
((ngx_http_log_ctx_t *) log->data)->current_request = r


+#if (NGX_THREADS)
+typedef struct ngx_thread_pool_s ngx_thread_pool_t;
+
+ngx_thread_pool_t *ngx_http_request_get_thread_pool(ngx_http_request_t *r);
+#endif
+
#endif /* _NGX_HTTP_REQUEST_H_INCLUDED_ */
diff --git src/os/unix/ngx_files.c src/os/unix/ngx_files.c
index 482d3276..3260c2aa 100644
--- src/os/unix/ngx_files.c
+++ src/os/unix/ngx_files.c
@@ -88,15 +88,94 @@ typedef struct {

size_t nbytes;
ngx_err_t err;
+} ngx_thread_file_read_ctx_t;
+
+typedef struct {
+ ngx_str_t name;
+ ngx_pool_t *pool;
+ ngx_open_file_info_t of;
+ ngx_int_t rv;
+} ngx_thread_file_open_ctx_t;
+
+typedef struct {
+ union {
+ ngx_thread_file_open_ctx_t open;
+ ngx_thread_file_read_ctx_t read;
+ };
} ngx_thread_file_ctx_t;


+static void
+ngx_thread_open_handler(void *data, ngx_log_t *log)
+{
+ ngx_int_t rc;
+ ngx_open_file_info_t *of;
+ ngx_thread_file_open_ctx_t *ctx;
+
+ ngx_log_debug0(NGX_LOG_DEBUG_CORE, log, 0, "thread open handler");
+
+ ctx = data;
+ of = &ctx->of;
+
+ of->fd = NGX_INVALID_FILE;
+
+ rc = ngx_open_and_stat_file(&ctx->name, of, log);
+ if (rc != NGX_OK && of->err == NGX_OK) {
+ of->err = rc;
+ }
+}
+
+
+ngx_int_t
+ngx_thread_open(ngx_file_t *file, ngx_open_file_info_t *of,
+ ngx_pool_t *pool)
+{
+ ngx_thread_task_t *task;
+ ngx_thread_file_open_ctx_t *ctx;
+
+ ngx_log_debug1(NGX_LOG_DEBUG_CORE, file->log, 0,
+ "thread open: %s", file->name.data);
+
+ task = file->thread_task;
+
+ if (task == NULL) {
+ task = ngx_thread_task_alloc(pool, sizeof(ngx_thread_file_ctx_t));
+ if (task == NULL) {
+ return NGX_ERROR;
+ }
+
+ file->thread_task = task;
+ }
+
+ ctx = task->ctx;
+
+ if (task->event.complete) {
+ task->event.complete = 0;
+ *of = ctx->of;
+
+ return of->err;
+ }
+
+ task->handler = ngx_thread_open_handler;
+
+ ctx->pool = pool;
+ ctx->name = file->name;
+ ctx->of = *of;
+
+ if (file->thread_handler(task, file) != NGX_OK) {
+ return NGX_ERROR;
+ }
+
+ return NGX_AGAIN;
+}
+
+
ssize_t
ngx_thread_read(ngx_file_t *file, u_char *buf, size_t size, off_t offset,
ngx_pool_t *pool)
{
ngx_thread_task_t *task;
- ngx_thread_file_ctx_t *ctx;
+ ngx_thread_file_read_ctx_t *ctx;

ngx_log_debug4(NGX_LOG_DEBUG_CORE, file->log, 0,
"thread read: %d, %p, %uz, %O",
@@ -155,7 +234,7 @@ ngx_thread_read(ngx_file_t *file, u_char *buf,
size_t size, off_t offset,
static void
ngx_thread_read_handler(void *data, ngx_log_t *log)
{
- ngx_thread_file_ctx_t *ctx = data;
+ ngx_thread_file_read_ctx_t *ctx = data;

ssize_t n;

@@ -478,7 +557,7 @@ ngx_thread_write_chain_to_file(ngx_file_t *file,
ngx_chain_t *cl, off_t offset,
ngx_pool_t *pool)
{
ngx_thread_task_t *task;
- ngx_thread_file_ctx_t *ctx;
+ ngx_thread_file_read_ctx_t *ctx;

ngx_log_debug3(NGX_LOG_DEBUG_CORE, file->log, 0,
"thread write chain: %d, %p, %O",
@@ -488,7 +567,7 @@ ngx_thread_write_chain_to_file(ngx_file_t *file,
ngx_chain_t *cl, off_t offset,

if (task == NULL) {
task = ngx_thread_task_alloc(pool,
- sizeof(ngx_thread_file_ctx_t));
+ sizeof(ngx_thread_file_read_ctx_t));
if (task == NULL) {
return NGX_ERROR;
}
@@ -536,7 +615,7 @@ ngx_thread_write_chain_to_file(ngx_file_t *file,
ngx_chain_t *cl, off_t offset,
static void
ngx_thread_write_chain_to_file_handler(void *data, ngx_log_t *log)
{
- ngx_thread_file_ctx_t *ctx = data;
+ ngx_thread_file_read_ctx_t *ctx = data;

#if (NGX_HAVE_PWRITEV)

diff --git src/os/unix/ngx_files.h src/os/unix/ngx_files.h
index 07872b13..2659c0cb 100644
--- src/os/unix/ngx_files.h
+++ src/os/unix/ngx_files.h
@@ -12,11 +12,11 @@
#include <ngx_config.h>
#include <ngx_core.h>

-
typedef int ngx_fd_t;
typedef struct stat ngx_file_info_t;
typedef ino_t ngx_file_uniq_t;

+#include <ngx_open_file_cache.h>

typedef struct {
u_char *name;
@@ -385,6 +385,8 @@ extern ngx_uint_t ngx_file_aio;
#endif

#if (NGX_THREADS)
+ngx_int_t ngx_thread_open(ngx_file_t *file, ngx_open_file_info_t *of,
+ ngx_pool_t *pool);
ssize_t ngx_thread_read(ngx_file_t *file, u_char *buf, size_t size,
off_t offset, ngx_pool_t *pool);
ssize_t ngx_thread_write_chain_to_file(ngx_file_t *file, ngx_chain_t *cl,
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel