From 104fc4be11e4429edcb81d39c2299433b71c54f6 Mon Sep 17 00:00:00 2001 From: Vito Caputo Date: Wed, 2 Dec 2020 22:11:23 -0800 Subject: [PATCH] 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. --- src/journal/journal-file.c | 7 +++--- src/journal/journal-file.h | 1 - src/journal/journal-verify.c | 8 +++---- src/journal/mmap-cache.c | 43 ++++++++++++++++------------------- src/journal/mmap-cache.h | 3 +-- src/journal/test-mmap-cache.c | 12 +++++----- 6 files changed, 33 insertions(+), 41 deletions(-) diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c index 1dbe81849c..2e56cecbca 100644 --- 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 3bdf551287..263d032b90 100644 --- 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 6ea2f4c898..4abaec9caa 100644 --- 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 788b232114..91edfe3505 100644 --- 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 43dc67f080..5b5e493cf2 100644 --- 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 d1d28768f5..606ce641ca 100644 --- 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);