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.

diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
index 2e56cecbcacadeed29a88721f7e96a56b5ae7bca..bcd932554d49a8e8d8c9adadbc375241214de5b9 100644 (file)
--- 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 4abaec9caae2c7758d919b36e650fcb9dec30590..5ecce518b29325fe9ef7ea4e16796b21d822629f 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, 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 91edfe35059b6e5284a87adda45bcc04d61709f3..9e0be01d41fa1befea61071388e69478558da150 100644 (file)
--- 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 5b5e493cf2c3cdc1136656b3c0e84f49dedd05e6..705e56e1f6ddf04cd46e89716f416c53970322ff 100644 (file)
--- 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 606ce641ca67b503a5eb978b939d3a0490035ddb..c3212fe179130214dc4275f181e1f6b1adee904f 100644 (file)
--- 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);