cache: move open to thread pool

Ka-Hing Cheung kahing at cloudflare.com
Mon Aug 27 21:38:36 UTC 2018


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

> - The code bypasses open file cache, and uses a direct call
>  in the http cache code instead.  While it might be ok in your
>  setup, it looks like an incomplete solution from the generic point
>  of view.  A better solution would be to introduce a generic
>  interface in ngx_open_cached_file() to allow use of thread
>  pools.

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

> - The code calls ngx_open_and_stat_file() whithin a thread, which
>  is relatively complex function never designed to be thread safe.
>  While it looks like currently it is, a better solution would be to
>  introduce a separate simple function to execute within a thread,
>  similar to ngx_thread_read_handler().

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

commit 3c67f1d972404bda7713839186a91cb18dbc96be
Author: Ka-Hing Cheung <kahing at 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,


More information about the nginx-devel mailing list