On Mon, Sep 3, 2018 at 9:09 AM, Maxim Dounin <mdou...@mdounin.ru> wrote:
> The biggest problem with in-place solution is that it requires
> in-place coding for all the places where we want to use aio_open.
> Currently we use ngx_open_cached_file() to open files everywhere,
> and it would be a good idea to preserve it as a starting point for
> all file open operations.
>
> This should be beneficial even if aio_open is only used when open
> file cache is actually disabled. And I'm actually fine with
> implementing this only when open file cache is disabled - but
> implementing this in ngx_open_cached_file(), to make it easier to
> add more places which use aio_open, and also makeing it possible
> to further improve things by making it compatible with
> open_file_cache.
The most difficult thing to switch to threaded open isn't which method
to call, but to expect NGX_AGAIN and have the necessary bookkeeping in
place to resume, so imo this is a pretty weak argument.
>
> Adding a code path somewhere in ngx_open_file_wrapper() should be
> simple enough. Note that probably there is no need to cover
> stat() as well, as well as to address various symlinks cases - I
> think it would be fine if aio_open will only work when there is no
> need to disable symlinks, and will only work for open(), as stat()
> after an open is expected to be fast.
A problem is that someone need to own the thread task (which for other
aio is owned by ngx_file, and someone needs to own the ngx_file).
ngx_open_cached_file doesn't care about anything beyond the file name.
That can of course change, but changing API is subjective and
difficult for me as an external contributor to decide on. I can try to
find time to do the change if you want to comment on how it should
look like.
>> > - 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.
>
> The problem is that ngx_open_and_stat_file() is never coded to be
> thread safe, and this is expected to cause problems sooner or
switched to use ngx_open_file_wrapper
>
> While this looks slightly better than in the previous iteration,
> this still leaves a lot to be desired. In particular, this still
> uses duplicate "rv == NGX_DECLINED" checks.
I poked around at it and thinks this one line duplication is the
cleanest. ngx_http_file_cache_read can also return DECLINED and
cleaning up nginx return code seems to be beyond the scope of this
work.
>
>> + 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;
>
> Please keep things in a single structure instead. This will allow
> to avoid many coresponding changes in the file.
Is 72 bytes per request not worth saving?
>> 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>
>>
>
> This introduces a dependency of low-level code from high-level
> OS-independant structures in ngx_open_file_cache.h. Instead, some
> low-level interface should be used.
Do you have a more concrete suggestion?
Addressed the other style and extraneous changes.
- Ka-Hing
commit f20eb2dc3f60c4360cc13101d15e94e5471027b6
Author: Ka-Hing Cheung <kah...@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..36da0218 100644
--- src/core/ngx_open_file_cache.c
+++ src/core/ngx_open_file_cache.c
@@ -30,9 +30,6 @@ static ngx_int_t ngx_file_o_path_info(ngx_fd_t fd,
ngx_file_info_t *fi,
ngx_log_t *log);
#endif
#endif
-static ngx_fd_t ngx_open_file_wrapper(ngx_str_t *name,
- ngx_open_file_info_t *of, ngx_int_t mode, ngx_int_t create,
- 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,
@@ -610,7 +607,7 @@ ngx_file_o_path_info(ngx_fd_t fd, ngx_file_info_t
*fi, ngx_log_t *log)
#endif /* NGX_HAVE_OPENAT */
-static ngx_fd_t
+ngx_fd_t
ngx_open_file_wrapper(ngx_str_t *name, ngx_open_file_info_t *of,
ngx_int_t mode, ngx_int_t create, ngx_int_t access, ngx_log_t *log)
{
diff --git src/core/ngx_open_file_cache.h src/core/ngx_open_file_cache.h
index d119c129..9c1c6bdc 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_fd_t ngx_open_file_wrapper(ngx_str_t *name, ngx_open_file_info_t *of,
+ ngx_int_t mode, ngx_int_t create, ngx_int_t access, 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..894ac70b 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,114 @@ 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;
+ 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);
+
+ ngx_memzero(&of, sizeof(of));
+
+ 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)
{
@@ -775,6 +840,8 @@ ngx_http_cache_thread_handler(ngx_thread_task_t
*task, ngx_file_t *file)
"thread pool \"%V\" not found", &name);
return NGX_ERROR;
}
+
+ clcf->thread_pool = tp;
}
task->event.data = r;
diff --git src/os/unix/ngx_files.c src/os/unix/ngx_files.c
index 482d3276..286f7d7f 100644
--- src/os/unix/ngx_files.c
+++ src/os/unix/ngx_files.c
@@ -88,15 +88,88 @@ typedef struct {
size_t nbytes;
ngx_err_t err;
+} ngx_thread_file_read_ctx_t;
+
+typedef struct {
+ ngx_str_t name;
+ 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_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_open_file_wrapper(&ctx->name, of,
+ NGX_FILE_RDONLY|NGX_FILE_NONBLOCK,
+ NGX_FILE_OPEN, 0, log);
+}
+
+
+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->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 +228,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 +551,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 +561,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 +609,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..c7b826e7 100644
--- src/os/unix/ngx_files.h
+++ src/os/unix/ngx_files.h
@@ -17,6 +17,7 @@ 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 +386,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
