Discussion:
cache: move open to thread pool
Ka-Hing Cheung via nginx-devel
2018-08-07 21:26:01 UTC
Permalink
commit 0bcfefa040abdff674047c15701d237ff16a5f2b
Author: Ka-Hing Cheung <***@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 has the side effect of disabling nginx's open_file_cache when
"aio threads" is turned on. For us we found that to have no impact
(probably because we have way too many files for open file cache to
be effective anyway).

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.

We wrote more about it here:
https://blog.cloudflare.com/how-we-scaled-nginx-and-saved-the-world-54-years-every-day/

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..94b4d552 100644
--- src/core/ngx_open_file_cache.h
+++ src/core/ngx_open_file_cache.h
@@ -124,6 +124,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..2910f6a1 100644
--- src/http/ngx_http_cache.h
+++ src/http/ngx_http_cache.h
@@ -114,6 +114,7 @@ struct ngx_http_cache_s {
unsigned exists:1;
unsigned temp_file:1;
unsigned purged:1;
+ unsigned opening:1;
unsigned reading:1;
unsigned secondary:1;
unsigned background:1;
diff --git src/http/ngx_http_file_cache.c src/http/ngx_http_file_cache.c
index 56866fa4..9cb110bf 100644
--- src/http/ngx_http_file_cache.c
+++ src/http/ngx_http_file_cache.c
@@ -18,6 +18,10 @@ 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 ngx_int_t ngx_http_file_cache_do_aio_open(ngx_http_request_t *r,
+ ngx_http_cache_t *c, ngx_open_file_info_t *of, 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 +272,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 +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);
+ }
+
if (c->reading) {
return ngx_http_file_cache_read(r, c);
}
@@ -343,23 +349,33 @@ ngx_http_file_cache_open(ngx_http_request_t *r)
goto done;
}

- clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
+ return ngx_http_file_cache_aio_open(r, c, &rv);

- ngx_memzero(&of, sizeof(ngx_open_file_info_t));
+done:

- 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 (rv == NGX_DECLINED) {
+ return ngx_http_file_cache_lock(r, c);
+ }

- if (ngx_open_cached_file(clcf->open_file_cache, &c->file.name,
&of, r->pool)
- != NGX_OK)
- {
- switch (of.err) {
+ return rv;
+}
+
+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 = c->file_cache;

- case 0:
+ rc = ngx_http_file_cache_do_aio_open(r, c, &of, rv);
+ if (rc == NGX_AGAIN) {
+ return rc;
+ }
+
+ if (rc != NGX_OK) {
+ switch (of.err) {
+ case NGX_OK:
return NGX_ERROR;

case NGX_ENOENT:
@@ -391,14 +407,13 @@ ngx_http_file_cache_open(ngx_http_request_t *r)

done:

- if (rv == NGX_DECLINED) {
+ if (*rv == NGX_DECLINED) {
return ngx_http_file_cache_lock(r, c);
}

- return rv;
+ return *rv;
}

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


+#if (NGX_THREADS)
+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;
+
+static void
+ngx_http_file_cache_thread_open_handler(void *data, ngx_log_t *log)
+{
+ ngx_thread_file_open_ctx_t *ctx = data;
+ ngx_pool_t *pool = ctx->pool;
+ ngx_open_file_info_t *of = &ctx->of;
+ ngx_pool_cleanup_t *cln;
+ ngx_pool_cleanup_file_t *clnf;
+ ngx_int_t rc;
+
+ ngx_log_debug0(NGX_LOG_DEBUG_CORE, log, 0, "thread read handler");
+
+ cln = ngx_pool_cleanup_add(pool, sizeof(ngx_pool_cleanup_file_t));
+ if (cln == NULL) {
+ ctx->of.err = NGX_ERROR;
+ return;
+ }
+
+ of->fd = NGX_INVALID_FILE;
+
+ rc = ngx_open_and_stat_file(&ctx->name, of, pool->log);
+ if (rc == NGX_OK && !of->is_dir) {
+ cln->handler = ngx_pool_cleanup_file;
+ clnf = cln->data;
+
+ clnf->fd = of->fd;
+ clnf->name = ctx->name.data;
+ clnf->log = pool->log;
+ }
+}
+
+
+static ngx_int_t
+ngx_http_file_cache_thread_open(ngx_http_cache_t *c, ngx_open_file_info_t *of,
+ ngx_int_t *rv, ngx_pool_t *pool)
+{
+ ngx_thread_task_t *task;
+ ngx_thread_file_open_ctx_t *ctx;
+ ngx_file_t *file = &c->file;
+
+ task = file->thread_task;
+
+ if (task == NULL) {
+ task = ngx_thread_task_alloc(pool, sizeof(ngx_thread_file_open_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;
+ *rv = ctx->rv;
+ return of->err;
+ }
+
+ task->handler = ngx_http_file_cache_thread_open_handler;
+
+ ctx->pool = pool;
+ ctx->name = file->name;
+ ctx->of = *of;
+ ctx->rv = *rv;
+
+ if (file->thread_handler(task, file) != NGX_OK) {
+ return NGX_ERROR;
+ }
+
+ return NGX_AGAIN;
+}
+#endif
+
+static ngx_int_t
+ngx_http_file_cache_do_aio_open(ngx_http_request_t *r, ngx_http_cache_t *c,
+ ngx_open_file_info_t *of, ngx_int_t *rv)
+{
+ ngx_http_core_loc_conf_t *clcf;
+
+ 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_THREADS)
+ if (clcf->aio == NGX_HTTP_AIO_THREADS) {
+ ngx_int_t rc;
+
+ c->file.thread_task = c->thread_task;
+ c->file.thread_handler = ngx_http_cache_thread_handler;
+ c->file.thread_ctx = r;
+
+ rc = ngx_http_file_cache_thread_open(c, of, rv, r->pool);
+
+ c->thread_task = c->file.thread_task;
+ c->opening = (rc == NGX_AGAIN);
+
+ return rc;
+ }
+#endif
+
+ return ngx_open_cached_file(clcf->open_file_cache, &c->file.name,
of, r->pool);
+}
+
static ssize_t
ngx_http_file_cache_aio_read(ngx_http_request_t *r, ngx_http_cache_t *c)
{
Maxim Dounin
2018-08-08 18:16:53 UTC
Permalink
Hello!
Post by Ka-Hing Cheung via nginx-devel
commit 0bcfefa040abdff674047c15701d237ff16a5f2b
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 has the side effect of disabling nginx's open_file_cache when
"aio threads" is turned on. For us we found that to have no impact
(probably because we have way too many files for open file cache to
be effective anyway).
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.
https://blog.cloudflare.com/how-we-scaled-nginx-and-saved-the-world-54-years-every-day/
First of all, thank you for your work. The numbers provided in
the article looks promising.

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.

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

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

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

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

- Also there are various style issues, including wrong
capitalization of the commit log summary line, strange commit
log indentation and header, missing empty lines. Please take a
look at http://nginx.org/en/docs/contributing_changes.html for
various hints. In particular, you may want to use Mercurial to
submit patches.

See also below for some in-line comments.
Post by Ka-Hing Cheung via nginx-devel
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..94b4d552 100644
--- src/core/ngx_open_file_cache.h
+++ src/core/ngx_open_file_cache.h
@@ -124,6 +124,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..2910f6a1 100644
--- src/http/ngx_http_cache.h
+++ src/http/ngx_http_cache.h
@@ -114,6 +114,7 @@ struct ngx_http_cache_s {
unsigned exists:1;
unsigned temp_file:1;
unsigned purged:1;
+ unsigned opening:1;
unsigned reading:1;
unsigned secondary:1;
unsigned background:1;
diff --git src/http/ngx_http_file_cache.c src/http/ngx_http_file_cache.c
index 56866fa4..9cb110bf 100644
--- src/http/ngx_http_file_cache.c
+++ src/http/ngx_http_file_cache.c
@@ -18,6 +18,10 @@ 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 ngx_int_t ngx_http_file_cache_do_aio_open(ngx_http_request_t *r,
+ ngx_http_cache_t *c, ngx_open_file_info_t *of, 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 +272,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 +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.
Post by Ka-Hing Cheung via nginx-devel
if (c->reading) {
return ngx_http_file_cache_read(r, c);
}
@@ -343,23 +349,33 @@ ngx_http_file_cache_open(ngx_http_request_t *r)
goto done;
}
- clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
+ return ngx_http_file_cache_aio_open(r, c, &rv);
- 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 (rv == NGX_DECLINED) {
+ return ngx_http_file_cache_lock(r, c);
+ }
- if (ngx_open_cached_file(clcf->open_file_cache, &c->file.name,
&of, r->pool)
- != NGX_OK)
- {
- switch (of.err) {
+ return rv;
+}
+
+static ngx_int_t
Style: there should be two empty lines between functions.
Post by Ka-Hing Cheung via nginx-devel
+ngx_http_file_cache_aio_open(ngx_http_request_t *r, ngx_http_cache_t *c,
+ ngx_int_t *rv)
Style: function arguments should be indented with 4 spaces.
Post by Ka-Hing Cheung via nginx-devel
+{
+ ngx_int_t rc;
+ ngx_open_file_info_t of;
+ ngx_http_file_cache_t *cache = c->file_cache;
+ rc = ngx_http_file_cache_do_aio_open(r, c, &of, rv);
+ if (rc == NGX_AGAIN) {
+ return rc;
+ }
+
+ if (rc != NGX_OK) {
+ switch (of.err) {
return NGX_ERROR;
@@ -391,14 +407,13 @@ ngx_http_file_cache_open(ngx_http_request_t *r)
- if (rv == NGX_DECLINED) {
+ if (*rv == NGX_DECLINED) {
return ngx_http_file_cache_lock(r, c);
}
- return rv;
+ return *rv;
}
-
static ngx_int_t
ngx_http_file_cache_lock(ngx_http_request_t *r, ngx_http_cache_t *c)
{
@@ -663,6 +678,127 @@ ngx_http_file_cache_read(ngx_http_request_t *r,
ngx_http_cache_t *c)
}
+#if (NGX_THREADS)
+typedef struct {
Style: as long as #if contains something complex, it should be
separated with empty lines.
Post by Ka-Hing Cheung via nginx-devel
+ ngx_str_t name;
+ ngx_pool_t *pool;
+ ngx_open_file_info_t of;
+ ngx_int_t rv;
+} ngx_thread_file_open_ctx_t;
+
+static void
+ngx_http_file_cache_thread_open_handler(void *data, ngx_log_t *log)
+{
+ ngx_thread_file_open_ctx_t *ctx = data;
+ ngx_pool_t *pool = ctx->pool;
+ ngx_open_file_info_t *of = &ctx->of;
+ ngx_pool_cleanup_t *cln;
+ ngx_pool_cleanup_file_t *clnf;
+ ngx_int_t rc;
Style: variable types should be sorted from shortest to longest;
variable names should be separated with two spaces from the
longest type; assignments should be written separately, except
some specific cases where one assignment is allowed in a separate
variables block.
Post by Ka-Hing Cheung via nginx-devel
+
+ ngx_log_debug0(NGX_LOG_DEBUG_CORE, log, 0, "thread read handler");
The "read" here is incorrect, should be "open".
Post by Ka-Hing Cheung via nginx-devel
+
+ cln = ngx_pool_cleanup_add(pool, sizeof(ngx_pool_cleanup_file_t));
+ if (cln == NULL) {
+ ctx->of.err = NGX_ERROR;
+ return;
+ }
The code is executed in a thread, and using request pool here is
not allowed.
Post by Ka-Hing Cheung via nginx-devel
+
+ of->fd = NGX_INVALID_FILE;
+
+ rc = ngx_open_and_stat_file(&ctx->name, of, pool->log);
+ if (rc == NGX_OK && !of->is_dir) {
+ cln->handler = ngx_pool_cleanup_file;
+ clnf = cln->data;
+
+ clnf->fd = of->fd;
+ clnf->name = ctx->name.data;
+ clnf->log = pool->log;
+ }
+}
+
+
+static ngx_int_t
+ngx_http_file_cache_thread_open(ngx_http_cache_t *c, ngx_open_file_info_t *of,
+ ngx_int_t *rv, ngx_pool_t *pool)
+{
+ ngx_thread_task_t *task;
+ ngx_thread_file_open_ctx_t *ctx;
+ ngx_file_t *file = &c->file;
Style, see above.
Post by Ka-Hing Cheung via nginx-devel
+
+ task = file->thread_task;
+
+ if (task == NULL) {
+ task = ngx_thread_task_alloc(pool, sizeof(ngx_thread_file_open_ctx_t));
+ if (task == NULL) {
+ return NGX_ERROR;
+ }
+
+ file->thread_task = task;
+ }
See above, this needs some guarantee that what's stored in
file->thread_task is compatible with all operations.
Post by Ka-Hing Cheung via nginx-devel
+
+ ctx = task->ctx;
+
+ if (task->event.complete) {
+ task->event.complete = 0;
+
+ *of = ctx->of;
+ *rv = ctx->rv;
+ return of->err;
+ }
+
+ task->handler = ngx_http_file_cache_thread_open_handler;
+
+ ctx->pool = pool;
+ ctx->name = file->name;
+ ctx->of = *of;
+ ctx->rv = *rv;
+
+ if (file->thread_handler(task, file) != NGX_OK) {
+ return NGX_ERROR;
+ }
+
+ return NGX_AGAIN;
+}
+#endif
+
+static ngx_int_t
Style: there should be an empty line before "#endif", and two
empty lines between "#endif" and the next function.
Post by Ka-Hing Cheung via nginx-devel
+ngx_http_file_cache_do_aio_open(ngx_http_request_t *r, ngx_http_cache_t *c,
+ ngx_open_file_info_t *of, ngx_int_t *rv)
+{
+ ngx_http_core_loc_conf_t *clcf;
+
+ 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_THREADS)
+ if (clcf->aio == NGX_HTTP_AIO_THREADS) {
+ ngx_int_t rc;
Style: there should be two spaces between a type and a variable
name; there should be an empty line after #if (and before #endif).
Post by Ka-Hing Cheung via nginx-devel
+
+ c->file.thread_task = c->thread_task;
+ c->file.thread_handler = ngx_http_cache_thread_handler;
+ c->file.thread_ctx = r;
+
+ rc = ngx_http_file_cache_thread_open(c, of, rv, r->pool);
+
+ c->thread_task = c->file.thread_task;
+ c->opening = (rc == NGX_AGAIN);
+
+ return rc;
+ }
+#endif
+
+ return ngx_open_cached_file(clcf->open_file_cache, &c->file.name,
of, r->pool);
Style: maximum text width is 80 characters.
Post by Ka-Hing Cheung via nginx-devel
+}
+
static ssize_t
ngx_http_file_cache_aio_read(ngx_http_request_t *r, ngx_http_cache_t *c)
{
_______________________________________________
nginx-devel mailing list
http://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Maxim Dounin
http://mdounin.ru/
Eran Kornblau
2018-08-08 19:44:20 UTC
Permalink
Hi,
Post by Maxim Dounin
- 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.
A small comment on this - I wrote such an implementation a while ago
in my module. We've been using it on production for the last ~3 years.

Code is here -
https://github.com/kaltura/nginx-vod-module/blob/master/ngx_async_open_file_cache.c

(There's a lot of code there that was copied from ngx_open_file_cache.c,
I did that since those functions are static, and didn't want to change nginx core.
If implemented as part of nginx core, all this duplication can be avoided...)

In my implementation, I added a function similar to ngx_open_cached_file -
(ngx_async_open_cached_file), that gets a few extra params -
1. The thread pool
2. The thread task (optional) - this was done in order to reuse the task
when opening multiple files in a single request
3. Callback + context - invoked once the async open completes

This function first checks the cache, if there's a hit, it returns synchronously
(NGX_OK).
Otherwise, it posts a task that does ngx_open_and_stat_file (NGX_AGAIN).
When the task completes, the main nginx thread updates the cache, and invokes
the user callback.

Hope this helps...

Eran
Ka-Hing Cheung via nginx-devel
2018-08-27 21:57:54 UTC
Permalink
Hi Eran,

Happy to see that we are not the only place where we find open to be a
problem. I took a look at your module and while the approach is sound,
I decided the complexity doesn't seem to be worth it. Hope to see some
numbers of how your module works at scale.

- Ka-Hing


On Wed, Aug 8, 2018 at 12:44 PM, Eran Kornblau
Post by Eran Kornblau
Hi,
Post by Maxim Dounin
- 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.
A small comment on this - I wrote such an implementation a while ago
in my module. We've been using it on production for the last ~3 years.
Code is here -
https://github.com/kaltura/nginx-vod-module/blob/master/ngx_async_open_file_cache.c
(There's a lot of code there that was copied from ngx_open_file_cache.c,
I did that since those functions are static, and didn't want to change nginx core.
If implemented as part of nginx core, all this duplication can be avoided...)
In my implementation, I added a function similar to ngx_open_cached_file -
(ngx_async_open_cached_file), that gets a few extra params -
1. The thread pool
2. The thread task (optional) - this was done in order to reuse the task
when opening multiple files in a single request
3. Callback + context - invoked once the async open completes
This function first checks the cache, if there's a hit, it returns synchronously
(NGX_OK).
Otherwise, it posts a task that does ngx_open_and_stat_file (NGX_AGAIN).
When the task completes, the main nginx thread updates the cache, and invokes
the user callback.
Hope this helps...
Eran
_______________________________________________
nginx-devel mailing list
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Eran Kornblau
2018-08-28 09:15:32 UTC
Permalink
Hi,
Post by Ka-Hing Cheung via nginx-devel
Hi Eran,
Happy to see that we are not the only place where we find open to be a problem. I took a look at your module and while the approach is sound, I decided the complexity doesn't seem to be worth it. Hope to see some numbers of how your module works at scale.
- Ka-Hing
I don't have numbers that show the improvement we got from moving open to a thread pool,
but what I can say is that on our environment, open can sometimes take an unreasonable
time to complete - several seconds.
I noticed it several years ago, after I added a feature of 'performance counters' to my module
(posted it here - https://forum.nginx.org/read.php?2,255847,258854#msg-258854).
This is the reason I decided to implement the async open - in order to avoid having all
worker active requests hang for so long.
In my case, I was already using open file cache, and while I got a fairly good hit ratio,
these extreme cases of worker hanging for few seconds wasn't something I wanted to accept.
Also, in my setup, I'm not using proxy_cache - my servers serve as origin servers behind a CDN,
so a solution that is limited to file cache is not something I could use.

Eran
Ka-Hing Cheung via nginx-devel
2018-08-09 21:43:19 UTC
Permalink
Very good feedback, we will look into implementing some of them at least.
Post by Maxim Dounin
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
- 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).
Post by Maxim Dounin
- 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.
Post by Maxim Dounin
- 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?
Post by Maxim Dounin
- 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
Post by Maxim Dounin
- 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.
Post by Maxim Dounin
Post by Ka-Hing Cheung via nginx-devel
@@ -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
Maxim Dounin
2018-08-10 11:39:46 UTC
Permalink
Hello!
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
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
- 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.

[...]
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
- 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.

[...]
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
- 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/
Ka-Hing Cheung via nginx-devel
2018-08-27 21:38:36 UTC
Permalink
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),
Post by Maxim Dounin
- 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.
Post by Maxim Dounin
- 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 <***@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,
Maxim Dounin
2018-09-03 16:09:03 UTC
Permalink
Hello!
Post by Ka-Hing Cheung via nginx-devel
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),
Post by Maxim Dounin
- 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 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.

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.
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
- 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
later - as we've seen in the previous version of your patch, which
tried to use pool allocations.

A separate simple function is needed to avoid such problems - and
to make it explicitly obvious that the code is expected to be
thread safe.
Post by Ka-Hing Cheung via nginx-devel
commit 3c67f1d972404bda7713839186a91cb18dbc96be
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
+
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.
Post by Ka-Hing Cheung via nginx-devel
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) {
-
- return NGX_ERROR;
-
- goto done;
-
- 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);
-
-
if (rv == NGX_DECLINED) {
return ngx_http_file_cache_lock(r, c);
}
return rv;
}
-
static ngx_int_t
Unrelated change and a style issue. As previously pointed out,
there should be two empty lines between functions.
Post by Ka-Hing Cheung via nginx-devel
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 };
Style: assignments should be written separately, except some
specific cases where one assignment is allowed in a separate
variables block.
Post by Ka-Hing Cheung via nginx-devel
+ 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) {
Style: as long as #if contains something complex, it should be
separated with empty lines.

Style: the "&&" shouldn't be left hanging at the end of line, it
should be wrapped to the next line instead.

Style: as long as "if" condition is on multiple lines, "{" should
be on its own line.
Post by Ka-Hing Cheung via nginx-devel
+
+ 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)
+#endif
As previously suggested, all this logic should be hidden in the
ngx_open_cached_file() function instead. You may want to take a
look at how it is done in ngx_write_chain_to_temp_file() for an
example.
Post by Ka-Hing Cheung via nginx-devel
+
+ if (rc != NGX_OK) {
+ switch (of.err) {
+ return NGX_ERROR;
+ goto done;
+
+ ngx_log_error(NGX_LOG_CRIT, r->connection->log, of.err,
+ ngx_open_file_n " \"%s\" failed", c->file.name.data);
+ return NGX_ERROR;
+ }
Style: you may want to take a look at the original code to find
out how to put additional empty lines.
Post by Ka-Hing Cheung via nginx-devel
+ }
+
+ 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);
+
+ if (rv == NGX_DECLINED) {
+ return ngx_http_file_cache_lock(r, c);
+ }
+ return rv;
Style: see above, this needs more empty lines.

Also, it might be a good idea to keep the "rv == NGX_DECLINED" /
ngx_http_file_cache_lock() processing in a single place.
Post by Ka-Hing Cheung via nginx-devel
+}
+
static ssize_t
Style: as previously pointed out, there should be two empty lines between
functions.
Post by Ka-Hing Cheung via nginx-devel
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;
This change looks unrelated, as ngx_http_request_get_thread_pool()
is never used elsewhere in this patch.
Post by Ka-Hing Cheung via nginx-devel
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;
As previously explained, you cannot use the request pool in a
thread. As such, this field is not needed.
Post by Ka-Hing Cheung via nginx-devel
+ 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.
Post by Ka-Hing Cheung via nginx-devel
+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>
-
Unrelated (and an incorrect from style point of view) change.
Post by Ka-Hing Cheung via nginx-devel
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.
Post by Ka-Hing Cheung via nginx-devel
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
http://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Maxim Dounin
http://mdounin.ru/
Ka-Hing Cheung via nginx-devel
2018-09-04 23:58:05 UTC
Permalink
Post by Maxim Dounin
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.
Post by Maxim Dounin
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.
Post by Maxim Dounin
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
- 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
Post by Maxim Dounin
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.
Post by Maxim Dounin
Post by Ka-Hing Cheung via nginx-devel
+ 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?
Post by Maxim Dounin
Post by Ka-Hing Cheung via nginx-devel
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 <***@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,
Maxim Dounin
2018-09-13 18:10:23 UTC
Permalink
Hello!
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
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.
Sure. But adding an alternative interface and dozens of lines of
the otherwise non-needed code everywhere we want to support
aio_open is something we can easily avoid, and we should do it.
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
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 ngx_open_file_cache() function takes a ngx_open_file_info_t
structure with various arguments, and it would be logical to
extend this structure to keep thread task as well.
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
- 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
You probably didn't get the point. You shouldn't use existing
functions - all them were not designed to be thread safe.
Instead, write a simple function using only low-level calls. Please
take a look at ngx_thread_read_handler() for an example.
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
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.
We can live with some duplication, but the whole thing needs to
be improved somehow to be more readable. The thing which bugs me
most is an opaque "c->open_rv" field, which is also passed as a
separate argument into the ngx_http_file_cache_aio_open()
function. Using some more descriptive name and saving it always
to avoid separate argument might be a way to go.
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
Post by Ka-Hing Cheung via nginx-devel
+ 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?
To save bytes, consider re-thinking the data used for open (and
you anyway has to do it when re-implementing the in-thread code as
a simple function using only low-level calls). Just a file name,
fd, and a return code should be enough here - and most of these
fields are already available in the existing ngx_thread_file_ctx_t
structure.
Post by Ka-Hing Cheung via nginx-devel
Post by Maxim Dounin
Post by Ka-Hing Cheung via nginx-devel
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?
See above.

[...]
Post by Ka-Hing Cheung via nginx-devel
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_
This looks like an unrelated change.

[...]
Post by Ka-Hing Cheung via nginx-devel
return rv;
}
-
static ngx_int_t
ngx_http_file_cache_lock(ngx_http_request_t *r, ngx_http_cache_t *c)
{
As previously pointed out, this is an unrelated change and a
style issue.

[...]
--
Maxim Dounin
http://mdounin.ru/
Roman Arutyunyan
2018-10-04 09:32:26 UTC
Permalink
Hi,
Post by Maxim Dounin
Hello!
[..]

Here's another approach to thread open. This time it's 4 patches:

- #1 a small open file cache refactoring
- #2 thread open in open file cache
- #3 thread open in http static module
- #4 thread open in http file cache
--
Roman Arutyunyan
Roman Arutyunyan
2018-11-01 12:30:47 UTC
Permalink
Hi,
Post by Roman Arutyunyan
Hi,
Post by Maxim Dounin
Hello!
[..]
- #1 a small open file cache refactoring
- #2 thread open in open file cache
- #3 thread open in http static module
- #4 thread open in http file cache
The next iteration of the work.
Only 3 patches this time.
--
Roman Arutyunyan
Maxim Konovalov
2018-11-01 16:08:10 UTC
Permalink
Post by Roman Arutyunyan
Hi,
Post by Roman Arutyunyan
Hi,
Post by Maxim Dounin
Hello!
[..]
- #1 a small open file cache refactoring
- #2 thread open in open file cache
- #3 thread open in http static module
- #4 thread open in http file cache
The next iteration of the work.
Only 3 patches this time.
Testing and feedbacks are welcome.
--
Maxim Konovalov
Maxim Konovalov
2018-11-08 14:19:19 UTC
Permalink
Hi Ka-Hing,

would you mind to test Roman's most recent patches that add
"aio_open" directive?

http://mailman.nginx.org/pipermail/nginx-devel/2018-November/011538.html

We are looking for overall performance and stability metrics and
feedback.

Much appreciated,

Maxim
--
Maxim Konovalov
Ka-Hing Cheung via nginx-devel
2018-11-15 22:53:46 UTC
Permalink
Hi,

I didn't forget about this. I am pretty swamped at the moment and
there's a holiday freeze coming up. Will get to his in December.

- Ka-Hing
Post by Maxim Konovalov
Hi Ka-Hing,
would you mind to test Roman's most recent patches that add
"aio_open" directive?
http://mailman.nginx.org/pipermail/nginx-devel/2018-November/011538.html
We are looking for overall performance and stability metrics and
feedback.
Much appreciated,
Maxim
--
Maxim Konovalov
Maxim Konovalov
2018-11-16 09:10:31 UTC
Permalink
Thanks, Ka-Hing, we'll wait for your feedback.
Post by Ka-Hing Cheung via nginx-devel
Hi,
I didn't forget about this. I am pretty swamped at the moment and
there's a holiday freeze coming up. Will get to his in December.
- Ka-Hing
Post by Maxim Konovalov
Hi Ka-Hing,
would you mind to test Roman's most recent patches that add
"aio_open" directive?
http://mailman.nginx.org/pipermail/nginx-devel/2018-November/011538.html
We are looking for overall performance and stability metrics and
feedback.
Much appreciated,
Maxim
--
Maxim Konovalov
--
Maxim Konovalov
Loading...