picnoir
/
Systemd
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

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.
This commit is contained in:
Vito Caputo 2020-12-06 00:21:17 -08:00 committed by Luca Boccassi
parent 52fc66635d
commit 258190a0d5
5 changed files with 25 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
src/journal/journal-file.c
View File

@ -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

2
src/journal/journal-verify.c
View File

@ -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;

24
src/journal/mmap-cache.c
View File

@ -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) {

3
src/journal/mmap-cache.h
View File

@ -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);

10
src/journal/test-mmap-cache.c
View File

@ -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);