[PATCH 1 of 2] Mp4: fixed directio usage with open_file_cache
# HG changeset patch
# User Maxim Dounin
# Date 1747276031 -10800
# Thu May 15 05:27:11 2025 +0300
# Node ID 6f0485388f31ead0177fd294f4883b59dfb1dea7
# Parent 4c872940b19b8181d791eb80c17110364150af9d
Mp4: fixed directio usage with open_file_cache.
With open_file_cache, if directio is enabled on a file descriptor after
opening the file, other consumers won't know about directio being enabled
and will use unaligned reads, leading to EINVAL errors from pread()
on Linux.
Further, if a file descriptor with directio enabled will be returned to
the mp4 module itself, during mp4 file processing it will be used in
assumption that directio is not enabled.
Fix is to use of.directio and ngx_open_cached_file() to enable directio,
and switch it off during mp4 file processing.
While this does some unneeded syscalls if the file is actually just opened
and we have to parse the file, this shouldn't be significant compared to
other mp4 file processing costs.
Note well that even with this fix using directio with open_file_cache
might be problematic on Linux if combined with file AIO or threaded IO,
as directio might be re-enabled after an unaligned read when a thread
tries to do an unaligned read for another request.
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
@@ -479,7 +479,7 @@ ngx_http_mp4_handler(ngx_http_request_t
u_char*last;
size_t root;
ngx_int_t rc, start, end;
-ngx_uint_t level, length;
+ngx_uint_t level, length, directio;
ngx_str_t path, value;
ngx_log_t *log;
ngx_buf_t *b;
@@ -519,7 +519,7 @@ ngx_http_mp4_handler(ngx_http_request_t
ngx_memzero(&of, sizeof(ngx_open_file_info_t));
of.read_ahead = clcf->read_ahead;
-of.directio = NGX_MAX_OFF_T_VALUE;
+of.directio = clcf->directio;
of.valid = clcf->open_file_cache_valid;
of.min_uses = clcf->open_file_cache_min_uses;
of.errors = clcf->open_file_cache_errors;
@@ -579,6 +579,7 @@ ngx_http_mp4_handler(ngx_http_request_t
start = -1;
length = 0;
+directio = 0;
r->headers_out.content_length_n = of.size;
mp4 = NULL;
b = NULL;
@@ -614,6 +615,21 @@ ngx_http_mp4_handler(ngx_http_request_t
if (start >= 0) {
r->single_range = 1;
+if (of.is_directio) {
+
+/*
+ * DIRECTIO is set on transfer only
+ * to allow kernel to cache "moov" atom
+ */
+
+if (ngx_directio_off(of.fd) == NGX_FILE_ERROR) {
+ngx_log_error(NGX_LOG_ALERT, log, ngx_errno,
+ ngx_directio_off_n " \"%s\" failed", path.data);
+}
+
+directio = 1;
+}
+
mp4 = ngx_pcalloc(r->pool, sizeof(ngx_http_mp4_file_t));
if (mp4 == NULL) {
return NGX_HTTP_INTERNAL_SERVER_ERROR;
@@ -622,6 +638,7 @@ ngx_http_mp4_handler(ngx_http_request_t
mp4->file.fd = of.fd;
mp4->file.name = path;
mp4->file.log = r->connection->log;
+mp4->file.directio = of.is_directio;
mp4->end = of.size;
mp4->start = (ngx_uint_t) start;
mp4->length = length;
@@ -656,23 +673,14 @@ ngx_http_mp4_handler(ngx_http_request_t
log->action = "sending mp4 to client";
-if (clcf->directio <= of.size) {
-
-/*
- * DIRECTIO is set on transfer only
- * to allow kernel to cache "moov" atom
- */
+if (directio) {
+
+/* DIRECTIO was switched off, restore it */
if (ngx_directio_on(of.fd) == NGX_FILE_ERROR) {
ngx_log_error(NGX_LOG_ALERT, log, ngx_errno,
ngx_directio_on_n " \"%s\" failed", path.data);
}
-
-of.is_directio = 1;
-
-if (mp4) {
-mp4->file.directio = 1;
-}
}
r->headers_out.status = NGX_HTTP_OK;
[PATCH 2 of 2] Mp4: of.directio_off flag to avoid unneeded syscalls
# HG changeset patch
# User Maxim Dounin
# Date 1747276516 -10800
# Thu May 15 05:35:16 2025 +0300
# Node ID abc30f01c381567949fcb62c8e3ce04f47cf7198
# Parent 6f0485388f31ead0177fd294f4883b59dfb1dea7
Mp4: of.directio_off flag to avoid unneeded syscalls.
When set, ngx_open_cached_file() won't try to enable directio on the file,
allowing the caller to do it itself, but will set is_directio flag in
ngx_open_file_info_t and in open file cache. The fact that directio needs
to be enabled by the caller is signalled in the of.is_directio_off output
flag.
This approach makes it possible to avoid unneeded syscalls when directio
is enabled, yet the caller needs it disabled for some initial file
processing, such as in the mp4 module.
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
@@ -249,6 +249,7 @@ ngx_open_cached_file(ngx_open_file_cache
of->is_link = file->is_link;
of->is_exec = file->is_exec;
of->is_directio = file->is_directio;
+of->is_directio_off = 0;
if (!file->is_dir) {
file->count++;
@@ -313,6 +314,7 @@ ngx_open_cached_file(ngx_open_file_cache
}
of->is_directio = file->is_directio;
+of->is_directio_off = 0;
goto update;
}
@@ -920,7 +922,11 @@ ngx_open_and_stat_file(ngx_str_t *name,
}
if (of->directio <= ngx_file_size(&fi)) {
-if (ngx_directio_on(fd) == NGX_FILE_ERROR) {
+if (of->directio_off) {
+of->is_directio = 1;
+of->is_directio_off = 1;
+
+} else if (ngx_directio_on(fd) == NGX_FILE_ERROR) {
ngx_log_error(NGX_LOG_ALERT, log, ngx_errno,
ngx_directio_on_n " \"%V\" failed", name);
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
@@ -42,12 +42,14 @@ typedef struct {
unsigned log:1;
unsigned errors:1;
unsigned events:1;
+unsigned directio_off:1;
unsigned is_dir:1;
unsigned is_file:1;
unsigned is_link:1;
unsigned is_exec:1;
unsigned is_directio:1;
+unsigned is_directio_off:1;
} ngx_open_file_info_t;
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
@@ -479,7 +479,7 @@ ngx_http_mp4_handler(ngx_http_request_t
u_char*last;
size_t root;
ngx_int_t rc, start, end;
-ngx_uint_t level, length, directio;
+ngx_uint_t level, length;
ngx_str_t path, value;
ngx_log_t *log;
ngx_buf_t *b;
@@ -520,6 +520,7 @@ ngx_http_mp4_handler(ngx_http_request_t
of.read_ahead = clcf->read_ahead;
of.directio = clcf->directio;
+of.directio_off = 1;
of.valid = clcf->open_file_cache_valid;
of.min_uses = clcf->open_file_cache_min_uses;
of.errors = clcf->open_file_cache_errors;
@@ -579,7 +580,6 @@ ngx_http_mp4_handler(ngx_http_request_t
start = -1;
length = 0;
-directio = 0;
r->headers_out.content_length_n = of.size;
mp4 = NULL;
b = NULL;
@@ -615,7 +615,7 @@ ngx_http_mp4_handler(ngx_http_request_t
if (start >= 0) {
r->single_range = 1;
-if (of.is_directio) {
+if (of.is_directio && !of.is_directio_off) {
/*
* DIRECTIO is set on transfer only
@@ -627,7 +627,7 @@ ngx_http_mp4_handler(ngx_http_request_t
ngx_directio_off_n " \"%s\" failed", path.data);
}
-directio = 1;
+of.is_directio_off = 1;
}
mp4 = ngx_pcalloc(r->pool, sizeof(ngx_http_mp4_file_t));
@@ -673,7 +673,7 @@ ngx_http_mp4_handler(ngx_http_request_t
log->action = "sending mp4 to client";
-if (directio) {
+if (of.is_directio_off) {
/* DIRECTIO was switched off, restore it */
[PATCH] Cache: directio support when reading cache files
# HG changeset patch
# User Maxim Dounin
# Date 1747277255 -10800
# Thu May 15 05:47:35 2025 +0300
# Node ID 579fd22909a485c48d0780e764c02af0c44c3666
# Parent abc30f01c381567949fcb62c8e3ce04f47cf7198
Cache: directio support when reading cache files.
diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
--- a/src/http/ngx_http_file_cache.c
+++ b/src/http/ngx_http_file_cache.c
@@ -353,7 +353,7 @@ ngx_http_file_cache_open(ngx_http_reques
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.directio = clcf->directio;
of.read_ahead = clcf->read_ahead;
if (ngx_open_cached_file(clcf->open_file_cache, &c->file.name, &of,
r->pool)
@@ -380,13 +380,36 @@ ngx_http_file_cache_open(ngx_http_reques
c->file.fd = of.fd;
c->file.log = r->connection->log;
+c->file.directio = of.is_directio;
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;
+if (of.is_directio) {
+
+c->body_start = ngx_align(c->body_start, clcf->directio_alignment);
+
+c->buf = ngx_calloc_buf(r->pool);
+if (c->buf == NULL) {
+return NGX_ERROR;
+}
+
+c->buf->start = ngx_pmemalign(r->pool, c->body_start,
+ clcf->directio_alignment);
+if (c->buf->start == NULL) {
+return NGX_ERROR;
+}
+
+c->buf->pos = c->buf->start;
+c->buf->last = c->buf->start;
+c->buf->end = c->buf->start + c->body_start;
+c->buf->temporary = 1;
+
+} else {
+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);
@@ -1663,6 +1686,7 @@ ngx_http_cache_send(ngx_http_request_t *
b->file->fd = c->file.fd;
b->file->name = c->file.name;
b->file->log = r->connection->log;
+b->file->directio = c->file.directio;
out.buf = b;
out.next = NULL;
[PATCH] Tests: mp4 tests with directio and open_file_cache
# HG changeset patch
# User Maxim Dounin
# Date 1747275653 -10800
# Thu May 15 05:20:53 2025 +0300
# Node ID c5566713e4bed939c0f2bc4de443fbe49ae212c4
# Parent 0a913a10945b996bcdac073467bf7bc957ef716e
Tests: mp4 tests with directio and open_file_cache.
diff --git a/mp4_directio.t b/mp4_directio.t
new file mode 100644
--- /dev/null
+++ b/mp4_directio.t
@@ -0,0 +1,78 @@
+#!/usr/bin/perl
+
+# (C) Maxim Dounin
+
+# Test for directio support in mp4 module.
+
+###
+
+use warnings;
+use strict;
+
+use Test::More;
+
+BEGIN { use FindBin; chdir($FindBin::Bin); }
+
+use lib 'lib';
+use Test::Nginx;
+
+###
+
+select STDERR; $| = 1;
+select STDOUT; $| = 1;
+
+my $t = Test::Nginx->new()->has(qw/http mp4/)->has_daemon('ffmpeg')
+ ->write_file_expand('nginx.conf', <<'EOF');
+
+%%TEST_GLOBALS%%
+
+daemon off;
+
+events {
+}
+
+http {
+%%TEST_GLOBALS_HTTP%%
+
+server {
+listen 127.0.0.1:8080;
+server_name localhost;
+
+location / {
+mp4;
+open_file_cache max=10;
+directio 0;
+}
+}
+}
+
+EOF
+
+plan(skip_all => 'no lavfi')
+ unless grep /lavfi/, `ffmpeg -loglevel quiet -formats`;
+plan(skip_all => 'no libx264 or libopenh264')
+ unless grep /libx264|libopenh264/, `ffmpeg -loglevel quiet -encoders`;
+system('ffmpeg -nostdin -loglevel quiet -y '
+ . '-f lavfi -i testsrc=duration=10:size=320x200:rate=15 '
+ . '-g 15 -c:v h264 '
+ . "${\($t->testdir())}/test.mp4") == 0
+ or die "Can't create mp4 file: $!";
+
+$t->run()->plan(2);
+
+###
+
+# mp4 module uses unaligned reads while parsing mp4 file, though
+# failed to disable directio if a file with directio enabled was
+# returned from open file cache
+
+like(http_get('/test.mp4?start=1.0'), qr/ 200 /, 'mp4 directio first');
+
+TODO: {
+local $TODO = 'not yet' unless $t->has_version('1.29.0') or $^O ne 'linux';
+
+like(http_get('/test.mp4?start=1.0'), qr/ 200 /, 'mp4 directio cached');
+
+}
+
+###
