From f6548f053d29542d5f640d0d5bf2dca68e808c24 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 26 Sep 2023 06:05:21 +0900 Subject: [PATCH 1/2] sd-journal: drop redundant re-reading of entry array object This effectively reverts e562f131585fe6ae32b1f035ba48c1548d695259. In the loop of the generic_array_bisect(), the offset of the entry array object is unchanged, the object is read at the beginning of the loop, and we do not read any other entry array object. Hence, it is not necessary to re-read the object every time we use the object. --- src/libsystemd/sd-journal/journal-file.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/libsystemd/sd-journal/journal-file.c b/src/libsystemd/sd-journal/journal-file.c index 334a28f948..4091336c3d 100644 --- a/src/libsystemd/sd-journal/journal-file.c +++ b/src/libsystemd/sd-journal/journal-file.c @@ -2877,8 +2877,8 @@ enum { static int generic_array_bisect_one( JournalFile *f, - uint64_t a, /* offset of entry array object. */ - uint64_t i, /* index of the entry item we will test. */ + Object *array, /* entry array object */ + uint64_t i, /* index of the entry item in the array we will test. */ uint64_t needle, int (*test_object)(JournalFile *f, uint64_t p, uint64_t needle), direction_t direction, @@ -2886,21 +2886,17 @@ static int generic_array_bisect_one( uint64_t *right, uint64_t *ret_offset) { - Object *array; uint64_t p; int r; assert(f); + assert(array); assert(test_object); assert(left); assert(right); assert(*left <= i); assert(i <= *right); - r = journal_file_move_to_object(f, OBJECT_ENTRY_ARRAY, a, &array); - if (r < 0) - return r; - p = journal_file_entry_array_item(f, array, i); if (p <= 0) r = -EBADMSG; @@ -2998,7 +2994,7 @@ static int generic_array_bisect( return 0; right--; - r = generic_array_bisect_one(f, a, right, needle, test_object, direction, &left, &right, &lp); + r = generic_array_bisect_one(f, array, right, needle, test_object, direction, &left, &right, &lp); if (r == -ENOANO) { n = right; continue; @@ -3012,13 +3008,13 @@ static int generic_array_bisect( * neighbors of the last index we looked at. */ if (last_index > 0 && last_index - 1 < right) { - r = generic_array_bisect_one(f, a, last_index - 1, needle, test_object, direction, &left, &right, NULL); + r = generic_array_bisect_one(f, array, last_index - 1, needle, test_object, direction, &left, &right, NULL); if (r < 0 && r != -ENOANO) return r; } if (last_index < right) { - r = generic_array_bisect_one(f, a, last_index + 1, needle, test_object, direction, &left, &right, NULL); + r = generic_array_bisect_one(f, array, last_index + 1, needle, test_object, direction, &left, &right, NULL); if (r < 0 && r != -ENOANO) return r; } @@ -3035,7 +3031,7 @@ static int generic_array_bisect( assert(left < right); i = (left + right) / 2; - r = generic_array_bisect_one(f, a, i, needle, test_object, direction, &left, &right, NULL); + r = generic_array_bisect_one(f, array, i, needle, test_object, direction, &left, &right, NULL); if (r < 0 && r != -ENOANO) return r; } @@ -3065,10 +3061,6 @@ found: if (subtract_one && t == 0 && i == 0) return 0; - r = journal_file_move_to_object(f, OBJECT_ENTRY_ARRAY, a, &array); - if (r < 0) - return r; - p = journal_file_entry_array_item(f, array, 0); if (p <= 0) return -EBADMSG; From ab8f553d1e09088fb1f633e014299e7bf6c30c9e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Sep 2023 02:14:31 +0900 Subject: [PATCH 2/2] sd-journal: rework generic_array_bisect() - Rename generic_array_bisect_one() -> generic_array_bisect_step(), as there is also generic_array_bisect_plus_one(), so the original name is confusing. - Make generic_array_bisect_step() return TEST_GOTO_NEXT or TEST_GOTO_PREVIOUS when the current array does not contain any matching entries. - Make generic_array_bisect_step() symmetric with respect to the direction we are going to, except for the journal corruption handling. - Make generic_array_bisect_step() gracefully handle journal corruptions, so the corruption handling in the caller side can be mostly dropped. - Especially, when the last entry in an array is corrupted, previously we tried to find a valid entry sequentially from the end of the array, but now we anyway bisect the array. That should improve performance of reading corrupted journal files. - Return earlier when no entry linked to the chained array (n == 0). - Add many comments. No behavior change unless journal is corrupted. --- src/libsystemd/sd-journal/journal-file.c | 214 ++++++++++++++++------- test/units/testsuite-04.journal.sh | 2 +- 2 files changed, 152 insertions(+), 64 deletions(-) diff --git a/src/libsystemd/sd-journal/journal-file.c b/src/libsystemd/sd-journal/journal-file.c index 4091336c3d..a0a87f51fd 100644 --- a/src/libsystemd/sd-journal/journal-file.c +++ b/src/libsystemd/sd-journal/journal-file.c @@ -2870,21 +2870,25 @@ static int generic_array_get( } enum { - TEST_FOUND, - TEST_LEFT, - TEST_RIGHT + TEST_FOUND, /* The current object passes the test. */ + TEST_LEFT, /* The current object is in an earlier position, and the object we are looking + * for should exist in a later position. */ + TEST_RIGHT, /* The current object is in a later position, and the object we are looking for + * should exist in an earlier position. */ + TEST_GOTO_NEXT, /* No matching object exists in this array and earlier arrays, go to the next array. */ + TEST_GOTO_PREVIOUS, /* No matching object exists in this array and later arrays, go to the previous array. */ }; -static int generic_array_bisect_one( +static int generic_array_bisect_step( JournalFile *f, - Object *array, /* entry array object */ - uint64_t i, /* index of the entry item in the array we will test. */ + Object *array, /* entry array object */ + uint64_t i, /* index of the entry item in the array we will test. */ uint64_t needle, int (*test_object)(JournalFile *f, uint64_t p, uint64_t needle), direction_t direction, - uint64_t *left, - uint64_t *right, - uint64_t *ret_offset) { + uint64_t *m, /* The maximum number of the entries we will check in the array. */ + uint64_t *left, /* The index of the left boundary in the array. */ + uint64_t *right) { /* The index of the right boundary in the array. */ uint64_t p; int r; @@ -2892,10 +2896,12 @@ static int generic_array_bisect_one( assert(f); assert(array); assert(test_object); + assert(m); assert(left); assert(right); assert(*left <= i); assert(i <= *right); + assert(*right < *m); p = journal_file_entry_array_item(f, array, i); if (p <= 0) @@ -2904,52 +2910,80 @@ static int generic_array_bisect_one( r = test_object(f, p, needle); if (IN_SET(r, -EBADMSG, -EADDRNOTAVAIL)) { log_debug_errno(r, "Encountered invalid entry while bisecting, cutting algorithm short."); - *right = i; - return -ENOANO; /* recognizable error */ + + /* The first entry in the array is corrupted, let's go back to the previous array. */ + if (i == 0) + return TEST_GOTO_PREVIOUS; + + /* Otherwise, cutting the array short. So, here we limit the number of elements we will see + * in this array, and set the right boundary to the last possibly non-corrupted object. */ + *m = i; + *right = i - 1; + return TEST_RIGHT; } if (r < 0) return r; if (r == TEST_FOUND) + /* There may be multiple entries that match with the needle. When the direction is down, we + * need to find the first matching entry, hence the right boundary can be moved, but the left + * one cannot. Similarly, when the direction is up, we need to find the last matching entry, + * hence the left boundary can be moved, but the right one cannot. */ r = direction == DIRECTION_DOWN ? TEST_RIGHT : TEST_LEFT; - if (r == TEST_RIGHT) - *right = i; - else - *left = i + 1; + if (r == TEST_RIGHT) { + /* Currently, left --- needle --- i --- right, hence we can move the right boundary to i. */ + if (direction == DIRECTION_DOWN) + *right = i; + else { + if (i == 0) + return TEST_GOTO_PREVIOUS; + *right = i - 1; + } + } else { + /* Currently, left --- i --- needle --- right, hence we can move the left boundary to i. */ + if (direction == DIRECTION_DOWN) { + /* Note, here *m is always positive, as by the assertions at the beginning, we have + * 0 <= *left <= i <= *right < m */ + if (i == *m - 1) + return TEST_GOTO_NEXT; - if (ret_offset) - *ret_offset = p; + *left = i + 1; + } else + *left = i; + } return r; } static int generic_array_bisect( JournalFile *f, - uint64_t first, - uint64_t n, - uint64_t needle, - int (*test_object)(JournalFile *f, uint64_t p, uint64_t needle), + uint64_t first, /* The offset of the first entry array object in the chain. */ + uint64_t n, /* The total number of elements in the chain of the entry array. */ + uint64_t needle, /* The target value (e.g. seqnum, monotonic, realtime, ...). */ + int (*test_object)(JournalFile *f, + uint64_t p, /* the offset of the (data or entry) object that will be tested. */ + uint64_t needle), direction_t direction, - Object **ret_object, - uint64_t *ret_offset, - uint64_t *ret_idx) { + Object **ret_object, /* The found object. */ + uint64_t *ret_offset, /* The offset of the found object. */ + uint64_t *ret_idx) { /* The index of the found object counted from the beginning of the entry array chain. */ /* Given an entry array chain, this function finds the object "closest" to the given needle in the * chain, taking into account the provided direction. A function can be provided to determine how * an object is matched against the given needle. * * Given a journal file, the offset of an object and the needle, the test_object() function should - * return TEST_LEFT if the needle is located earlier in the entry array chain, TEST_LEFT if the - * needle is located later in the entry array chain and TEST_FOUND if the object matches the needle. + * return TEST_RIGHT if the needle is located earlier in the entry array chain, TEST_LEFT if the + * needle is located later in the entry array chain, and TEST_FOUND if the object matches the needle. * If test_object() returns TEST_FOUND for a specific object, that object's information will be used * to populate the return values of this function. If test_object() never returns TEST_FOUND, the * return values are populated with the details of one of the objects closest to the needle. If the * direction is DIRECTION_UP, the earlier object is used. Otherwise, the later object is used. - */ + * If there are multiple objects that test_object() return TEST_FOUND for, then the first matching + * object returned when direction is DIRECTION_DOWN. Otherwise the last object is returned. */ - uint64_t a, p, t = 0, i = 0, last_p = 0, last_index = UINT64_MAX; - bool subtract_one = false; + uint64_t a, p, t = 0, i, last_index = UINT64_MAX; ChainCacheItem *ci; Object *array; int r; @@ -2957,6 +2991,9 @@ static int generic_array_bisect( assert(f); assert(test_object); + if (n <= 0) + return 0; + /* Start with the first array in the chain */ a = first; @@ -2982,73 +3019,92 @@ static int generic_array_bisect( } while (a > 0) { - uint64_t left = 0, right, k, lp; + uint64_t left, right, k, m; r = journal_file_move_to_object(f, OBJECT_ENTRY_ARRAY, a, &array); if (r < 0) return r; k = journal_file_entry_array_n_items(f, array); - right = MIN(k, n); - if (right <= 0) + m = MIN(k, n); + if (m <= 0) return 0; - right--; - r = generic_array_bisect_one(f, array, right, needle, test_object, direction, &left, &right, &lp); - if (r == -ENOANO) { - n = right; - continue; + left = 0; + right = m - 1; + + if (direction == DIRECTION_UP) { + /* If we're going upwards, the last entry of the previous array may pass the test, + * and the first entry of the current array may not pass. In that case, the last + * entry of the previous array must be returned. Hence, we need to test the first + * entry of the current array. */ + r = generic_array_bisect_step(f, array, 0, needle, test_object, direction, &m, &left, &right); + if (r < 0) + return r; + if (r == TEST_GOTO_PREVIOUS) + goto previous; } + + /* Test the last entry of this array, to determine if we should go to the next array. */ + r = generic_array_bisect_step(f, array, right, needle, test_object, direction, &m, &left, &right); if (r < 0) return r; + if (r == TEST_GOTO_PREVIOUS) + goto previous; + /* The expected entry should be in this array, (or the last entry of the previous array). */ if (r == TEST_RIGHT) { + /* If we cached the last index we looked at, let's try to not to jump too wildly * around and see if we can limit the range to look at early to the immediate * neighbors of the last index we looked at. */ - if (last_index > 0 && last_index - 1 < right) { - r = generic_array_bisect_one(f, array, last_index - 1, needle, test_object, direction, &left, &right, NULL); - if (r < 0 && r != -ENOANO) + if (last_index > 0 && left < last_index - 1 && last_index - 1 < right) { + r = generic_array_bisect_step(f, array, last_index - 1, needle, test_object, direction, &m, &left, &right); + if (r < 0) return r; + if (r == TEST_GOTO_PREVIOUS) + goto previous; } - if (last_index < right) { - r = generic_array_bisect_one(f, array, last_index + 1, needle, test_object, direction, &left, &right, NULL); - if (r < 0 && r != -ENOANO) + if (last_index < UINT64_MAX && left < last_index + 1 && last_index + 1 < right) { + r = generic_array_bisect_step(f, array, last_index + 1, needle, test_object, direction, &m, &left, &right); + if (r < 0) return r; + if (r == TEST_GOTO_PREVIOUS) + goto previous; } for (;;) { if (left == right) { - if (direction == DIRECTION_UP) - subtract_one = true; - i = left; goto found; } assert(left < right); - i = (left + right) / 2; + i = (left + right + (direction == DIRECTION_UP)) / 2; - r = generic_array_bisect_one(f, array, i, needle, test_object, direction, &left, &right, NULL); - if (r < 0 && r != -ENOANO) + r = generic_array_bisect_step(f, array, i, needle, test_object, direction, &m, &left, &right); + if (r < 0) return r; + if (r == TEST_GOTO_PREVIOUS) + goto previous; } } + /* Not found in this array (or the last entry of this array should be returned), go to the next array. */ + assert(r == (direction == DIRECTION_DOWN ? TEST_GOTO_NEXT : TEST_LEFT)); + if (k >= n) { if (direction == DIRECTION_UP) { - i = n; - subtract_one = true; + assert(n > 0); + i = n - 1; goto found; } return 0; } - last_p = lp; - n -= k; t += k; last_index = UINT64_MAX; @@ -3057,23 +3113,55 @@ static int generic_array_bisect( return 0; -found: - if (subtract_one && t == 0 && i == 0) +previous: + /* Not found in the current array, return the last entry of the previous array. */ + assert(r == TEST_GOTO_PREVIOUS); + + /* The current array is the first in the chain. no previous array. */ + if (t == 0) return 0; + /* When we are going downwards, there is no matching entries in the previous array. */ + if (direction == DIRECTION_DOWN) + return 0; + + /* Indicate to go to the previous array later. Note, do not move to the previous array here, + * as that may invalidate the current array object in the mmap cache and + * journal_file_entry_array_item() below may read invalid address. */ + i = UINT64_MAX; + +found: p = journal_file_entry_array_item(f, array, 0); if (p <= 0) return -EBADMSG; /* Let's cache this item for the next invocation */ - chain_cache_put(f->chain_cache, ci, first, a, p, t, subtract_one ? (i > 0 ? i-1 : UINT64_MAX) : i); + chain_cache_put(f->chain_cache, ci, first, a, p, t, i); - if (subtract_one && i == 0) - p = last_p; - else if (subtract_one) - p = journal_file_entry_array_item(f, array, i - 1); - else - p = journal_file_entry_array_item(f, array, i); + if (i == UINT64_MAX) { + uint64_t m; + + /* Get the last entry of the previous array. */ + + r = bump_entry_array(f, NULL, a, first, DIRECTION_UP, &a); + if (r <= 0) + return r; + + r = journal_file_move_to_object(f, OBJECT_ENTRY_ARRAY, a, &array); + if (r < 0) + return r; + + m = journal_file_entry_array_n_items(f, array); + if (m == 0 || t < m) + return -EBADMSG; + + t -= m; + i = m - 1; + } + + p = journal_file_entry_array_item(f, array, i); + if (p == 0) + return -EBADMSG; if (ret_object) { r = journal_file_move_to_object(f, OBJECT_ENTRY, p, ret_object); @@ -3085,7 +3173,7 @@ found: *ret_offset = p; if (ret_idx) - *ret_idx = t + i + (subtract_one ? -1 : 0); + *ret_idx = t + i; return 1; } diff --git a/test/units/testsuite-04.journal.sh b/test/units/testsuite-04.journal.sh index 3efac616e0..6ea4cc4296 100755 --- a/test/units/testsuite-04.journal.sh +++ b/test/units/testsuite-04.journal.sh @@ -238,6 +238,6 @@ done < <(find /test-journals/no-rtc -name "*.zst") journalctl --directory="$JOURNAL_DIR" --list-boots --output=json >/tmp/lb1 diff -u /tmp/lb1 - <<'EOF' -[{"index":-3,"boot_id":"5ea5fc4f82a14186b5332a788ef9435e","first_entry":1666569600994371,"last_entry":1666584266223608},{"index":-2,"boot_id":"bea6864f21ad4c9594c04a99d89948b0","first_entry":1666569601005945,"last_entry":1666584347230411},{"index":-1,"boot_id":"4c708e1fd0744336be16f3931aa861fb","first_entry":1666569601017222,"last_entry":1666584354649355},{"index":0,"boot_id":"35e8501129134edd9df5267c49f744a4","first_entry":1666569601009823,"last_entry":1666584438086856}] +[{"index":-3,"boot_id":"5ea5fc4f82a14186b5332a788ef9435e","first_entry":1666569600994371,"last_entry":1666584266223608},{"index":-2,"boot_id":"bea6864f21ad4c9594c04a99d89948b0","first_entry":1666569601005945,"last_entry":1666584347230411},{"index":-1,"boot_id":"4c708e1fd0744336be16f3931aa861fb","first_entry":1666584293354007,"last_entry":1666584354649355},{"index":0,"boot_id":"35e8501129134edd9df5267c49f744a4","first_entry":1666569601009823,"last_entry":1666584438086856}] EOF rm -rf "$JOURNAL_DIR" /tmp/lb1