# HG changeset patch
# User Maxim Dounin <mdou...@mdounin.ru>
# Date 1748208437 -10800
# Mon May 26 00:27:17 2025 +0300
# Node ID e3e07705481df3c60a3bac8af9101323ee7ceb83
# Parent 6032949667f1e5fda9ce364d6eb1668074274c56
Open file cached: correct directio handling with threads.
On Linux with open_file_cache and directio enabled along with threaded IO,
if a file descriptor is shared among multiple requests, one request might
unexpectedly switch directio on while another request does an unaligned
read in a thread, leading to EINVAL from pread().
To fix this, now all directio changes are done with reference counting
when open file cache is used, and directio is only re-enabled on a file
descriptor when there are no outstanding requests to switch it off.
diff --git a/src/core/ngx_open_file_cache.c b/src/core/ngx_open_file_cache.c
--- a/src/core/ngx_open_file_cache.c
+++ b/src/core/ngx_open_file_cache.c
@@ -41,7 +41,8 @@ static void ngx_open_file_add_event(ngx_
ngx_cached_open_file_t *file, ngx_open_file_info_t *of, ngx_log_t *log);
static void ngx_open_file_cleanup(void *data);
static void ngx_close_cached_file(ngx_open_file_cache_t *cache,
- ngx_cached_open_file_t *file, ngx_uint_t min_uses, ngx_log_t *log);
+ ngx_cached_open_file_t *file, ngx_uint_t min_uses, ngx_uint_t directio_off,
+ ngx_log_t *log);
static void ngx_open_file_del_event(ngx_cached_open_file_t *file);
static void ngx_expire_old_cached_files(ngx_open_file_cache_t *cache,
ngx_uint_t n, ngx_log_t *log);
@@ -51,6 +52,8 @@ static ngx_cached_open_file_t *
ngx_open_file_lookup(ngx_open_file_cache_t *cache, ngx_str_t *name,
uint32_t hash);
static void ngx_open_file_cache_remove(ngx_event_t *ev);
+static ngx_open_file_cache_cleanup_t *
+ ngx_open_file_cache_get_cleanup(ngx_pool_t *p, ngx_fd_t fd);
ngx_open_file_cache_t *
@@ -118,7 +121,7 @@ ngx_open_file_cache_cleanup(void *data)
if (!file->err && !file->is_dir) {
file->close = 1;
file->count = 0;
- ngx_close_cached_file(cache, file, 0, ngx_cycle->log);
+ ngx_close_cached_file(cache, file, 0, 0, ngx_cycle->log);
} else {
ngx_free(file->name);
@@ -396,6 +399,7 @@ create:
file->count = 0;
file->use_event = 0;
file->event = NULL;
+ file->directio_off = of->is_directio_off;
add_event:
@@ -449,6 +453,7 @@ found:
ofcln->cache = cache;
ofcln->file = file;
ofcln->min_uses = of->min_uses;
+ ofcln->directio_off = of->is_directio_off;
ofcln->log = pool->log;
}
@@ -1032,7 +1037,8 @@ ngx_open_file_cleanup(void *data)
c->file->count--;
- ngx_close_cached_file(c->cache, c->file, c->min_uses, c->log);
+ ngx_close_cached_file(c->cache, c->file, c->min_uses, c->directio_off,
+ c->log);
/* drop one or two expired open files */
ngx_expire_old_cached_files(c->cache, 1, c->log);
@@ -1041,12 +1047,25 @@ ngx_open_file_cleanup(void *data)
static void
ngx_close_cached_file(ngx_open_file_cache_t *cache,
- ngx_cached_open_file_t *file, ngx_uint_t min_uses, ngx_log_t *log)
+ ngx_cached_open_file_t *file, ngx_uint_t min_uses, ngx_uint_t directio_off,
+ ngx_log_t *log)
{
ngx_log_debug5(NGX_LOG_DEBUG_CORE, log, 0,
"close cached open file: %s, fd:%d, c:%d, u:%d, %d",
file->name, file->fd, file->count, file->uses, file->close);
+ if (directio_off) {
+ file->directio_off--;
+
+ if (file->directio_off == 0) {
+ if (ngx_directio_on(file->fd) == NGX_FILE_ERROR) {
+ ngx_log_error(NGX_LOG_ALERT, log, ngx_errno,
+ ngx_directio_on_n " \"%s\" failed",
+ file->name);
+ }
+ }
+ }
+
if (!file->close) {
file->accessed = ngx_time();
@@ -1143,7 +1162,7 @@ ngx_expire_old_cached_files(ngx_open_fil
if (!file->err && !file->is_dir) {
file->close = 1;
- ngx_close_cached_file(cache, file, 0, log);
+ ngx_close_cached_file(cache, file, 0, 0, log);
} else {
ngx_free(file->name);
@@ -1255,10 +1274,92 @@ ngx_open_file_cache_remove(ngx_event_t *
file->close = 1;
- ngx_close_cached_file(fev->cache, file, 0, ev->log);
+ ngx_close_cached_file(fev->cache, file, 0, 0, ev->log);
/* free memory only when fev->cache and fev->file are already not needed */
ngx_free(ev->data);
ngx_free(ev);
}
+
+
+ngx_int_t
+ngx_open_file_directio_on(ngx_fd_t fd, ngx_pool_t *pool)
+{
+ ngx_open_file_cache_cleanup_t *c;
+
+ /*
+ * DIRECTIO is only re-enabled on a file descriptor
+ * when there are no outstanding requests to switch it off
+ */
+
+ c = ngx_open_file_cache_get_cleanup(pool, fd);
+
+ if (c) {
+ if (!c->directio_off) {
+ return NGX_OK;
+ }
+
+ c->directio_off = 0;
+ c->file->directio_off--;
+
+ if (c->file->directio_off > 0) {
+ return NGX_OK;
+ }
+ }
+
+ if (ngx_directio_on(fd) == NGX_FILE_ERROR) {
+ return NGX_ERROR;
+ }
+
+ return NGX_OK;
+}
+
+
+ngx_int_t
+ngx_open_file_directio_off(ngx_fd_t fd, ngx_pool_t *pool)
+{
+ ngx_open_file_cache_cleanup_t *c;
+
+ c = ngx_open_file_cache_get_cleanup(pool, fd);
+
+ if (c) {
+ if (c->directio_off) {
+ return NGX_OK;
+ }
+
+ c->directio_off = 1;
+ c->file->directio_off++;
+
+ if (c->file->directio_off > 1) {
+ return NGX_OK;
+ }
+ }
+
+ if (ngx_directio_off(fd) == NGX_FILE_ERROR) {
+ return NGX_ERROR;
+ }
+
+ return NGX_OK;
+}
+
+
+static ngx_open_file_cache_cleanup_t *
+ngx_open_file_cache_get_cleanup(ngx_pool_t *p, ngx_fd_t fd)
+{
+ ngx_pool_cleanup_t *cln;
+ ngx_open_file_cache_cleanup_t *c;
+
+ for (cln = p->cleanup; cln; cln = cln->next) {
+ if (cln->handler == ngx_open_file_cleanup) {
+
+ c = cln->data;
+
+ if (c->file->fd == fd) {
+ return c;
+ }
+ }
+ }
+
+ return NULL;
+}
diff --git a/src/core/ngx_open_file_cache.h b/src/core/ngx_open_file_cache.h
--- a/src/core/ngx_open_file_cache.h
+++ b/src/core/ngx_open_file_cache.h
@@ -67,6 +67,8 @@ typedef struct {
off_t size;
ngx_err_t err;
+ ngx_uint_t directio_off;
+
uint32_t uses;
#if (NGX_HAVE_OPENAT)
@@ -103,6 +105,7 @@ typedef struct {
ngx_open_file_cache_t *cache;
ngx_cached_open_file_t *file;
ngx_uint_t min_uses;
+ ngx_uint_t directio_off;
ngx_log_t *log;
} ngx_open_file_cache_cleanup_t;
@@ -125,5 +128,8 @@ ngx_open_file_cache_t *ngx_open_file_cac
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_file_directio_on(ngx_fd_t fd, ngx_pool_t *pool);
+ngx_int_t ngx_open_file_directio_off(ngx_fd_t fd, ngx_pool_t *pool);
+
#endif /* _NGX_OPEN_FILE_CACHE_H_INCLUDED_ */
diff --git a/src/core/ngx_output_chain.c b/src/core/ngx_output_chain.c
--- a/src/core/ngx_output_chain.c
+++ b/src/core/ngx_output_chain.c
@@ -553,7 +553,9 @@ ngx_output_chain_copy_buf(ngx_output_cha
#if (NGX_HAVE_ALIGNED_DIRECTIO)
if (ctx->unaligned) {
- if (ngx_directio_off(src->file->fd) == NGX_FILE_ERROR) {
+ if (ngx_open_file_directio_off(src->file->fd, ctx->pool)
+ != NGX_OK)
+ {
ngx_log_error(NGX_LOG_ALERT, ctx->pool->log, ngx_errno,
ngx_directio_off_n " \"%s\" failed",
src->file->name.data);
@@ -600,7 +602,9 @@ ngx_output_chain_copy_buf(ngx_output_cha
err = ngx_errno;
- if (ngx_directio_on(src->file->fd) == NGX_FILE_ERROR) {
+ if (ngx_open_file_directio_on(src->file->fd, ctx->pool)
+ != NGX_OK)
+ {
ngx_log_error(NGX_LOG_ALERT, ctx->pool->log, ngx_errno,
ngx_directio_on_n " \"%s\" failed",
src->file->name.data);
diff --git a/src/http/modules/ngx_http_mp4_module.c
b/src/http/modules/ngx_http_mp4_module.c
--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -622,7 +622,7 @@ ngx_http_mp4_handler(ngx_http_request_t
* to allow kernel to cache "moov" atom
*/
- if (ngx_directio_off(of.fd) == NGX_FILE_ERROR) {
+ if (ngx_open_file_directio_off(of.fd, r->pool) != NGX_OK) {
ngx_log_error(NGX_LOG_ALERT, log, ngx_errno,
ngx_directio_off_n " \"%s\" failed", path.data);
}
@@ -677,7 +677,7 @@ ngx_http_mp4_handler(ngx_http_request_t
/* DIRECTIO was switched off, restore it */
- if (ngx_directio_on(of.fd) == NGX_FILE_ERROR) {
+ if (ngx_open_file_directio_on(of.fd, r->pool) != NGX_OK) {
ngx_log_error(NGX_LOG_ALERT, log, ngx_errno,
ngx_directio_on_n " \"%s\" failed", path.data);
}
