From 258190a0d5b2c1fe8ee50e3a9204d845947d4835 Mon Sep 17 00:00:00 2001 From: Vito Caputo Date: Sun, 6 Dec 2020 00:21:17 -0800 Subject: [PATCH] mmap-cache: drop ret_size from mmap_cache_get() The ret_size result is a bit of an awkward optimization that in a sense enables bypassing the mmap-cache API, while encouraging duplication of logic it already implements. It's only utilized in one place; journal_file_move_to_object(), apparently to avoid the overhead of remapping the whole object again once its header, and thus its actual size, is known. With mmap-cache's context cache, the overhead of simply re-getting the object with the now known size should already be negligible. So it's not clear what benefit this brings, unless avoiding some function calls that do very little in the hot context-cache hit case is of such a priority. There's value in having all object-sized gets pass through mmap_cache_get(), as it provides a single entrypoint for instrumentation in profiling/statistics gathering. When journal_file_move_to_object() bypasses getting the full object size, you don't capture the full picture on the mmap-cache side in terms of object sizes explicitly loaded from a journal file. I'd like to see additional accounting in mmap_cache_get() in a future commit, taking advantage of this change. --- src/journal/journal-file.c | 26 +++++++++++--------------- src/journal/journal-verify.c | 2 +- src/journal/mmap-cache.c | 24 +++++++----------------- src/journal/mmap-cache.h | 3 +-- src/journal/test-mmap-cache.c | 10 +++++----- 5 files changed, 25 insertions(+), 40 deletions(-) diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c index 2e56cecbca..bcd932554d 100644 --- a/src/journal/journal-file.c +++ b/src/journal/journal-file.c @@ -724,8 +724,7 @@ static int journal_file_move_to( bool keep_always, uint64_t offset, uint64_t size, - void **ret, - size_t *ret_size) { + void **ret) { int r; @@ -751,7 +750,7 @@ static int journal_file_move_to( return -EADDRNOTAVAIL; } - return mmap_cache_get(f->mmap, f->cache_fd, 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); } static uint64_t minimum_header_size(Object *o) { @@ -923,7 +922,6 @@ static int journal_file_check_object(JournalFile *f, uint64_t offset, Object *o) int journal_file_move_to_object(JournalFile *f, ObjectType type, uint64_t offset, Object **ret) { int r; void *t; - size_t tsize; Object *o; uint64_t s; @@ -942,7 +940,7 @@ int journal_file_move_to_object(JournalFile *f, ObjectType type, uint64_t offset "Attempt to move to object located in file header: %" PRIu64, offset); - r = journal_file_move_to(f, type, false, offset, sizeof(ObjectHeader), &t, &tsize); + r = journal_file_move_to(f, type, false, offset, sizeof(ObjectHeader), &t); if (r < 0) return r; @@ -973,13 +971,11 @@ int journal_file_move_to_object(JournalFile *f, ObjectType type, uint64_t offset "Attempt to move to object of unexpected type: %" PRIu64, offset); - if (s > tsize) { - r = journal_file_move_to(f, type, false, offset, s, &t, NULL); - if (r < 0) - return r; + r = journal_file_move_to(f, type, false, offset, s, &t); + if (r < 0) + return r; - o = (Object*) t; - } + o = (Object*) t; r = journal_file_check_object(f, offset, o); if (r < 0) @@ -1062,7 +1058,7 @@ int journal_file_append_object( if (r < 0) return r; - r = journal_file_move_to(f, type, false, p, size, &t, NULL); + r = journal_file_move_to(f, type, false, p, size, &t); if (r < 0) return r; @@ -1165,7 +1161,7 @@ int journal_file_map_data_hash_table(JournalFile *f) { OBJECT_DATA_HASH_TABLE, true, p, s, - &t, NULL); + &t); if (r < 0) return r; @@ -1191,7 +1187,7 @@ int journal_file_map_field_hash_table(JournalFile *f) { OBJECT_FIELD_HASH_TABLE, true, p, s, - &t, NULL); + &t); if (r < 0) return r; @@ -3510,7 +3506,7 @@ int journal_file_open( goto fail; } - r = mmap_cache_get(f->mmap, f->cache_fd, 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); 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-verify.c b/src/journal/journal-verify.c index 4abaec9caa..5ecce518b2 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, 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); if (r < 0) return r; diff --git a/src/journal/mmap-cache.c b/src/journal/mmap-cache.c index 91edfe3505..9e0be01d41 100644 --- a/src/journal/mmap-cache.c +++ b/src/journal/mmap-cache.c @@ -306,8 +306,7 @@ static int try_context( bool keep_always, uint64_t offset, size_t size, - void **ret, - size_t *ret_size) { + void **ret) { Context *c; @@ -339,8 +338,6 @@ static int try_context( c->window->keep_always = c->window->keep_always || keep_always; *ret = (uint8_t*) c->window->ptr + (offset - c->window->offset); - if (ret_size) - *ret_size = c->window->size - (offset - c->window->offset); return 1; } @@ -352,8 +349,7 @@ static int find_mmap( bool keep_always, uint64_t offset, size_t size, - void **ret, - size_t *ret_size) { + void **ret) { Window *w; Context *c; @@ -381,8 +377,6 @@ static int find_mmap( w->keep_always = w->keep_always || keep_always; *ret = (uint8_t*) w->ptr + (offset - w->offset); - if (ret_size) - *ret_size = w->size - (offset - w->offset); return 1; } @@ -422,8 +416,7 @@ static int add_mmap( uint64_t offset, size_t size, struct stat *st, - void **ret, - size_t *ret_size) { + void **ret) { uint64_t woffset, wsize; Context *c; @@ -481,8 +474,6 @@ static int add_mmap( context_attach_window(c, w); *ret = (uint8_t*) w->ptr + (offset - w->offset); - if (ret_size) - *ret_size = w->size - (offset - w->offset); return 1; @@ -499,8 +490,7 @@ int mmap_cache_get( uint64_t offset, size_t size, struct stat *st, - void **ret, - size_t *ret_size) { + void **ret) { int r; @@ -512,14 +502,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, context, keep_always, offset, size, ret, ret_size); + r = try_context(m, f, context, keep_always, offset, size, ret); if (r != 0) { m->n_context_cache_hit++; return r; } /* Search for a matching mmap */ - r = find_mmap(m, f, context, keep_always, offset, size, ret, ret_size); + r = find_mmap(m, f, context, keep_always, offset, size, ret); if (r != 0) { m->n_window_list_hit++; return r; @@ -528,7 +518,7 @@ int mmap_cache_get( m->n_missed++; /* Create a new mmap */ - return add_mmap(m, f, context, keep_always, offset, size, st, ret, ret_size); + return add_mmap(m, f, context, keep_always, offset, size, st, ret); } void mmap_cache_stats_log_debug(MMapCache *m) { diff --git a/src/journal/mmap-cache.h b/src/journal/mmap-cache.h index 5b5e493cf2..705e56e1f6 100644 --- a/src/journal/mmap-cache.h +++ b/src/journal/mmap-cache.h @@ -22,8 +22,7 @@ int mmap_cache_get( uint64_t offset, size_t size, struct stat *st, - void **ret, - size_t *ret_size); + void **ret); MMapFileDescriptor * mmap_cache_add_fd(MMapCache *m, int fd, int prot); void mmap_cache_free_fd(MMapCache *m, MMapFileDescriptor *f); diff --git a/src/journal/test-mmap-cache.c b/src/journal/test-mmap-cache.c index 606ce641ca..c3212fe179 100644 --- a/src/journal/test-mmap-cache.c +++ b/src/journal/test-mmap-cache.c @@ -34,23 +34,23 @@ int main(int argc, char *argv[]) { assert_se(z >= 0); unlink(pz); - r = mmap_cache_get(m, fx, 0, false, 1, 2, NULL, &p, NULL); + r = mmap_cache_get(m, fx, 0, false, 1, 2, NULL, &p); assert_se(r >= 0); - r = mmap_cache_get(m, fx, 0, false, 2, 2, NULL, &q, NULL); + r = mmap_cache_get(m, fx, 0, false, 2, 2, NULL, &q); assert_se(r >= 0); assert_se((uint8_t*) p + 1 == (uint8_t*) q); - r = mmap_cache_get(m, fx, 1, false, 3, 2, NULL, &q, NULL); + r = mmap_cache_get(m, fx, 1, false, 3, 2, NULL, &q); assert_se(r >= 0); assert_se((uint8_t*) p + 2 == (uint8_t*) q); - r = mmap_cache_get(m, fx, 0, false, 16ULL*1024ULL*1024ULL, 2, NULL, &p, NULL); + r = mmap_cache_get(m, fx, 0, false, 16ULL*1024ULL*1024ULL, 2, NULL, &p); assert_se(r >= 0); - r = mmap_cache_get(m, fx, 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); assert_se(r >= 0); assert_se((uint8_t*) p + 1 == (uint8_t*) q);