mmap-cache: bind prot(ection) to MMapFileDescriptor

There are no mmap_cache_get() users that actually deviate prot
from the JournalFile's f->prot.

So there's no point in making this a separate parameter to
mmap_cache_get(), nor is there any need to store it in
JournalFile's f->prot.

Instead just pass it to mmap_cache_add_fd() at MMapFileDescriptor
creation, storing it in there for the mmap() callers, which
already receive MMapFileDescriptor *.

For functions receiving both an MMapFileDescriptor * and prot,
the prot argument has been simply removed and call sites updated.

Formalizing this fd:prot binding at the public API also enables
discarding the prot check in window_matches(), which is a hot
function on long window lists, so a minor CPU efficiency gain
should be had there as seen with the past removal of the fd
check.  Unnoticable for uncached journals, but maybe a little
runtime improvement when cached in specific circumstances.

window_matches_fd() has also been simplified to treat the
MMapFileDescrptor * as equivalent to its fd and prot.

diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
index 1dbe81849c2dd662f77e76e62878cd0eeb0d0784..2e56cecbcacadeed29a88721f7e96a56b5ae7bca 100644 (file)
--- a/src/journal/journal-file.c
+++ b/src/journal/journal-file.c
@@ -751,7 +751,7 @@ static int journal_file_move_to(
                         return -EADDRNOTAVAIL;
         }
 
-        return mmap_cache_get(f->mmap, f->cache_fd, f->prot, type_to_context(type), keep_always, offset, size, &f->last_stat, ret, ret_size);
+        return mmap_cache_get(f->mmap, f->cache_fd, type_to_context(type), keep_always, offset, size, &f->last_stat, ret, ret_size);
 }
 
 static uint64_t minimum_header_size(Object *o) {
@@ -3365,7 +3365,6 @@ int journal_file_open(
                 .mode = mode,
 
                 .flags = flags,
-                .prot = prot_from_flags(flags),
                 .writable = (flags & O_ACCMODE) != O_RDONLY,
 
 #if HAVE_ZSTD
@@ -3464,7 +3463,7 @@ int journal_file_open(
                         goto fail;
         }
 
-        f->cache_fd = mmap_cache_add_fd(f->mmap, f->fd);
+        f->cache_fd = mmap_cache_add_fd(f->mmap, f->fd, prot_from_flags(flags));
         if (!f->cache_fd) {
                 r = -ENOMEM;
                 goto fail;
@@ -3511,7 +3510,7 @@ int journal_file_open(
                 goto fail;
         }
 
-        r = mmap_cache_get(f->mmap, f->cache_fd, f->prot, CONTEXT_HEADER, true, 0, PAGE_ALIGN(sizeof(Header)), &f->last_stat, &h, NULL);
+        r = mmap_cache_get(f->mmap, f->cache_fd, CONTEXT_HEADER, true, 0, PAGE_ALIGN(sizeof(Header)), &f->last_stat, &h, NULL);
         if (r == -EINVAL) {
                 /* Some file systems (jffs2 or p9fs) don't support mmap() properly (or only read-only
                  * mmap()), and return EINVAL in that case. Let's propagate that as a more recognizable error

diff --git a/src/journal/journal-file.h b/src/journal/journal-file.h
index 3bdf551287ba91eb360c7a5886ef19bf67a45ff8..263d032b90dd0b2a9c4fe17d950bb6597d40a39a 100644 (file)
--- a/src/journal/journal-file.h
+++ b/src/journal/journal-file.h
@@ -63,7 +63,6 @@ typedef struct JournalFile {
         mode_t mode;
 
         int flags;
-        int prot;
         bool writable:1;
         bool compress_xz:1;
         bool compress_lz4:1;

diff --git a/src/journal/journal-verify.c b/src/journal/journal-verify.c
index 6ea2f4c898a9b67ee3c6cdf57913a3ddee666516..4abaec9caae2c7758d919b36e650fcb9dec30590 100644 (file)
--- a/src/journal/journal-verify.c
+++ b/src/journal/journal-verify.c
@@ -377,7 +377,7 @@ static int contains_uint64(MMapCache *m, MMapFileDescriptor *f, uint64_t n, uint
 
                 c = (a + b) / 2;
 
-                r = mmap_cache_get(m, f, PROT_READ|PROT_WRITE, 0, false, c * sizeof(uint64_t), sizeof(uint64_t), NULL, (void **) &z, NULL);
+                r = mmap_cache_get(m, f, 0, false, c * sizeof(uint64_t), sizeof(uint64_t), NULL, (void **) &z, NULL);
                 if (r < 0)
                         return r;
 
@@ -862,19 +862,19 @@ int journal_file_verify(
                 goto fail;
         }
 
-        cache_data_fd = mmap_cache_add_fd(f->mmap, data_fd);
+        cache_data_fd = mmap_cache_add_fd(f->mmap, data_fd, PROT_READ|PROT_WRITE);
         if (!cache_data_fd) {
                 r = log_oom();
                 goto fail;
         }
 
-        cache_entry_fd = mmap_cache_add_fd(f->mmap, entry_fd);
+        cache_entry_fd = mmap_cache_add_fd(f->mmap, entry_fd, PROT_READ|PROT_WRITE);
         if (!cache_entry_fd) {
                 r = log_oom();
                 goto fail;
         }
 
-        cache_entry_array_fd = mmap_cache_add_fd(f->mmap, entry_array_fd);
+        cache_entry_array_fd = mmap_cache_add_fd(f->mmap, entry_array_fd, PROT_READ|PROT_WRITE);
         if (!cache_entry_array_fd) {
                 r = log_oom();
                 goto fail;

diff --git a/src/journal/mmap-cache.c b/src/journal/mmap-cache.c
index 788b232114d49a287c0b644d730f593066e8f1d2..91edfe35059b6e5284a87adda45bcc04d61709f3 100644 (file)
--- a/src/journal/mmap-cache.c
+++ b/src/journal/mmap-cache.c
@@ -25,7 +25,6 @@ struct Window {
         bool keep_always:1;
         bool in_unused:1;
 
-        int prot;
         void *ptr;
         uint64_t offset;
         size_t size;
@@ -49,6 +48,7 @@ struct Context {
 struct MMapFileDescriptor {
         MMapCache *cache;
         int fd;
+        int prot;
         bool sigbus;
         LIST_HEAD(Window, windows);
 };
@@ -112,6 +112,7 @@ static void window_unlink(Window *w) {
 
 static void window_invalidate(Window *w) {
         assert(w);
+        assert(w->fd);
 
         if (w->invalidated)
                 return;
@@ -121,7 +122,7 @@ static void window_invalidate(Window *w) {
          * trigger any further SIGBUS, possibly overrunning the sigbus
          * queue. */
 
-        assert_se(mmap(w->ptr, w->size, w->prot, MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED, -1, 0) == w->ptr);
+        assert_se(mmap(w->ptr, w->size, w->fd->prot, MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED, -1, 0) == w->ptr);
         w->invalidated = true;
 }
 
@@ -133,27 +134,25 @@ static void window_free(Window *w) {
         free(w);
 }
 
-_pure_ static bool window_matches(Window *w, int prot, uint64_t offset, size_t size) {
+_pure_ static bool window_matches(Window *w, uint64_t offset, size_t size) {
         assert(w);
         assert(size > 0);
 
         return
-                prot == w->prot &&
                 offset >= w->offset &&
                 offset + size <= w->offset + w->size;
 }
 
-_pure_ static bool window_matches_fd(Window *w, MMapFileDescriptor *f, int prot, uint64_t offset, size_t size) {
+_pure_ static bool window_matches_fd(Window *w, MMapFileDescriptor *f, uint64_t offset, size_t size) {
         assert(w);
         assert(f);
 
         return
-                w->fd &&
-                f->fd == w->fd->fd &&
-                window_matches(w, prot, offset, size);
+                w->fd == f &&
+                window_matches(w, offset, size);
 }
 
-static Window *window_add(MMapCache *m, MMapFileDescriptor *f, int prot, bool keep_always, uint64_t offset, size_t size, void *ptr) {
+static Window *window_add(MMapCache *m, MMapFileDescriptor *f, bool keep_always, uint64_t offset, size_t size, void *ptr) {
         Window *w;
 
         assert(m);
@@ -176,7 +175,6 @@ static Window *window_add(MMapCache *m, MMapFileDescriptor *f, int prot, bool ke
         *w = (Window) {
                 .cache = m,
                 .fd = f,
-                .prot = prot,
                 .keep_always = keep_always,
                 .offset = offset,
                 .size = size,
@@ -304,7 +302,6 @@ static int make_room(MMapCache *m) {
 static int try_context(
                 MMapCache *m,
                 MMapFileDescriptor *f,
-                int prot,
                 unsigned context,
                 bool keep_always,
                 uint64_t offset,
@@ -329,7 +326,7 @@ static int try_context(
         if (!c->window)
                 return 0;
 
-        if (!window_matches_fd(c->window, f, prot, offset, size)) {
+        if (!window_matches_fd(c->window, f, offset, size)) {
 
                 /* Drop the reference to the window, since it's unnecessary now */
                 context_detach_window(c);
@@ -351,7 +348,6 @@ static int try_context(
 static int find_mmap(
                 MMapCache *m,
                 MMapFileDescriptor *f,
-                int prot,
                 unsigned context,
                 bool keep_always,
                 uint64_t offset,
@@ -371,7 +367,7 @@ static int find_mmap(
                 return -EIO;
 
         LIST_FOREACH(by_fd, w, f->windows)
-                if (window_matches(w, prot, offset, size))
+                if (window_matches(w, offset, size))
                         break;
 
         if (!w)
@@ -391,7 +387,7 @@ static int find_mmap(
         return 1;
 }
 
-static int mmap_try_harder(MMapCache *m, void *addr, MMapFileDescriptor *f, int prot, int flags, uint64_t offset, size_t size, void **res) {
+static int mmap_try_harder(MMapCache *m, void *addr, MMapFileDescriptor *f, int flags, uint64_t offset, size_t size, void **res) {
         void *ptr;
 
         assert(m);
@@ -401,7 +397,7 @@ static int mmap_try_harder(MMapCache *m, void *addr, MMapFileDescriptor *f, int
         for (;;) {
                 int r;
 
-                ptr = mmap(addr, size, prot, flags, f->fd, offset);
+                ptr = mmap(addr, size, f->prot, flags, f->fd, offset);
                 if (ptr != MAP_FAILED)
                         break;
                 if (errno != ENOMEM)
@@ -421,7 +417,6 @@ static int mmap_try_harder(MMapCache *m, void *addr, MMapFileDescriptor *f, int
 static int add_mmap(
                 MMapCache *m,
                 MMapFileDescriptor *f,
-                int prot,
                 unsigned context,
                 bool keep_always,
                 uint64_t offset,
@@ -471,7 +466,7 @@ static int add_mmap(
                         wsize = PAGE_ALIGN(st->st_size - woffset);
         }
 
-        r = mmap_try_harder(m, NULL, f, prot, MAP_SHARED, woffset, wsize, &d);
+        r = mmap_try_harder(m, NULL, f, MAP_SHARED, woffset, wsize, &d);
         if (r < 0)
                 return r;
 
@@ -479,7 +474,7 @@ static int add_mmap(
         if (!c)
                 goto outofmem;
 
-        w = window_add(m, f, prot, keep_always, woffset, wsize, d);
+        w = window_add(m, f, keep_always, woffset, wsize, d);
         if (!w)
                 goto outofmem;
 
@@ -499,7 +494,6 @@ outofmem:
 int mmap_cache_get(
                 MMapCache *m,
                 MMapFileDescriptor *f,
-                int prot,
                 unsigned context,
                 bool keep_always,
                 uint64_t offset,
@@ -518,14 +512,14 @@ int mmap_cache_get(
         assert(context < MMAP_CACHE_MAX_CONTEXTS);
 
         /* Check whether the current context is the right one already */
-        r = try_context(m, f, prot, context, keep_always, offset, size, ret, ret_size);
+        r = try_context(m, f, context, keep_always, offset, size, ret, ret_size);
         if (r != 0) {
                 m->n_context_cache_hit++;
                 return r;
         }
 
         /* Search for a matching mmap */
-        r = find_mmap(m, f, prot, context, keep_always, offset, size, ret, ret_size);
+        r = find_mmap(m, f, context, keep_always, offset, size, ret, ret_size);
         if (r != 0) {
                 m->n_window_list_hit++;
                 return r;
@@ -534,7 +528,7 @@ int mmap_cache_get(
         m->n_missed++;
 
         /* Create a new mmap */
-        return add_mmap(m, f, prot, context, keep_always, offset, size, st, ret, ret_size);
+        return add_mmap(m, f, context, keep_always, offset, size, st, ret, ret_size);
 }
 
 void mmap_cache_stats_log_debug(MMapCache *m) {
@@ -614,7 +608,7 @@ bool mmap_cache_got_sigbus(MMapCache *m, MMapFileDescriptor *f) {
         return f->sigbus;
 }
 
-MMapFileDescriptor* mmap_cache_add_fd(MMapCache *m, int fd) {
+MMapFileDescriptor* mmap_cache_add_fd(MMapCache *m, int fd, int prot) {
         MMapFileDescriptor *f;
         int r;
 
@@ -635,6 +629,7 @@ MMapFileDescriptor* mmap_cache_add_fd(MMapCache *m, int fd) {
 
         f->cache = m;
         f->fd = fd;
+        f->prot = prot;
 
         r = hashmap_put(m->fds, FD_TO_PTR(fd), f);
         if (r < 0)

diff --git a/src/journal/mmap-cache.h b/src/journal/mmap-cache.h
index 43dc67f080bc30a8f0db00a6d281ebf339a5f6d8..5b5e493cf2c3cdc1136656b3c0e84f49dedd05e6 100644 (file)
--- a/src/journal/mmap-cache.h
+++ b/src/journal/mmap-cache.h
@@ -17,7 +17,6 @@ MMapCache* mmap_cache_unref(MMapCache *m);
 int mmap_cache_get(
         MMapCache *m,
         MMapFileDescriptor *f,
-        int prot,
         unsigned context,
         bool keep_always,
         uint64_t offset,
@@ -25,7 +24,7 @@ int mmap_cache_get(
         struct stat *st,
         void **ret,
         size_t *ret_size);
-MMapFileDescriptor * mmap_cache_add_fd(MMapCache *m, int fd);
+MMapFileDescriptor * mmap_cache_add_fd(MMapCache *m, int fd, int prot);
 void mmap_cache_free_fd(MMapCache *m, MMapFileDescriptor *f);
 
 void mmap_cache_stats_log_debug(MMapCache *m);

diff --git a/src/journal/test-mmap-cache.c b/src/journal/test-mmap-cache.c
index d1d28768f5b6e454d5c3aab517a7bb5bad693b03..606ce641ca67b503a5eb978b939d3a0490035ddb 100644 (file)
--- a/src/journal/test-mmap-cache.c
+++ b/src/journal/test-mmap-cache.c
@@ -24,7 +24,7 @@ int main(int argc, char *argv[]) {
         assert_se(x >= 0);
         unlink(px);
 
-        assert_se(fx = mmap_cache_add_fd(m, x));
+        assert_se(fx = mmap_cache_add_fd(m, x, PROT_READ));
 
         y = mkostemp_safe(py);
         assert_se(y >= 0);
@@ -34,23 +34,23 @@ int main(int argc, char *argv[]) {
         assert_se(z >= 0);
         unlink(pz);
 
-        r = mmap_cache_get(m, fx, PROT_READ, 0, false, 1, 2, NULL, &p, NULL);
+        r = mmap_cache_get(m, fx, 0, false, 1, 2, NULL, &p, NULL);
         assert_se(r >= 0);
 
-        r = mmap_cache_get(m, fx, PROT_READ, 0, false, 2, 2, NULL, &q, NULL);
+        r = mmap_cache_get(m, fx, 0, false, 2, 2, NULL, &q, NULL);
         assert_se(r >= 0);
 
         assert_se((uint8_t*) p + 1 == (uint8_t*) q);
 
-        r = mmap_cache_get(m, fx, PROT_READ, 1, false, 3, 2, NULL, &q, NULL);
+        r = mmap_cache_get(m, fx, 1, false, 3, 2, NULL, &q, NULL);
         assert_se(r >= 0);
 
         assert_se((uint8_t*) p + 2 == (uint8_t*) q);
 
-        r = mmap_cache_get(m, fx, PROT_READ, 0, false, 16ULL*1024ULL*1024ULL, 2, NULL, &p, NULL);
+        r = mmap_cache_get(m, fx, 0, false, 16ULL*1024ULL*1024ULL, 2, NULL, &p, NULL);
         assert_se(r >= 0);
 
-        r = mmap_cache_get(m, fx, PROT_READ, 1, false, 16ULL*1024ULL*1024ULL+1, 2, NULL, &q, NULL);
+        r = mmap_cache_get(m, fx, 1, false, 16ULL*1024ULL*1024ULL+1, 2, NULL, &q, NULL);
         assert_se(r >= 0);
 
         assert_se((uint8_t*) p + 1 == (uint8_t*) q);