Re: [systemd-devel] [PATCH v2] journalctl: Improve boot ID lookup
On Sat, 25.04.15 15:51, Jan Janssen (medhe...@web.de) wrote: Yeah, patches like these always do end up looking messy. It's much easier to read after applying it. Well, it jumps from one boot to the next boot using _BOOT_ID matches. It starts at the journal head to get the boot ID, makes a _BOOT_ID match and then comes from the opposite journal direction (tail) to get the end a boot. And then flushes the matches, and advances the journal from that exact position one further (which gives us the start and ID of our next boot). Rinse and repeat. Note, v1 differs in that it assumes sd_journal_flush_matches() will also reset the position we are in the journal at that moment. That version went around that by using a cursor and seeking to the after flushing. Hence why I wonder if this behavior of slush_matches is expected/desired or not. Yes, _flush_matches() should exclusively flush the matches and not reset the position. If it would change the position this would be a bug. Matches really only change how we look for the next entry, not how we look at the current one. I gave this another look today. Since journalctl uses SD_JOURNAL_LOCAL_ONLY by default, the new algorithm cannot trip up on interleaving boot IDs (since they shouldn't be interleaving in that case, per the above assumption). Same goes for --machine mode. Now, --file, --directory and --merge mode on the other hand does confuse the new algorithm. Yeah, I think using the seek to boot id logic only really makes sense for local journals. I think we should refuse operation if you mix --merge (or the related other switches) with it. But I think it might be worth it to go with my above suggestion if that'll be accepted. Alternatively, we could either refuse --boot and --list-boots in those cases, or ship the old algorithm along with the new one and use that one in those cases where the faster one gets confused. Or we stick with status quo and don't improve on the algorithm altogether. I'd like to know the option to go with, to ease me mind... I think your altered algorithm does make a ton of sense, but please add code that explicitly fails if you combine --boot with --merge and so on... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v2] journalctl: Improve boot ID lookup
On 2015-04-08 16:14, Jan Janssen wrote: On 2015-04-08 14:39, Lennart Poettering wrote: On Thu, 02.04.15 17:08, Jan Janssen (medhe...@web.de) wrote: This method should greatly improve offset based lookup. We now don't have to aggregate the full boot listing just so we can jump to a specific position, which can be a real pain on big journals just for a mere -b -1 case. As an additional benefit --list-boots should improve slightly too, because we now need to do less seeking. Note that there can be a change in boot order with this lookup method because it will use the order of boots in the journal, not the realtime stamp stored in them. That's arguably better, though. https://bugs.freedesktop.org/show_bug.cgi?id=72601 --- Hi, today I realized that it would be nice if we could do without the cursor seeking. Turns out we can! I could swear that I tested sd_journal_flush_matches() would reset our position in the journal. But it seems that sd_journal_next/previous will advance just fine from the last position we were in, even after a flush. Though, I would still like someone with better journal internals knowledge confirm that this is how it's supposed to work. Some testing/timing from others than me would be nice too. Hmm, the patch is hard to read, can you explain what precisely the new algorithm is you propose? Lennart Yeah, patches like these always do end up looking messy. It's much easier to read after applying it. Well, it jumps from one boot to the next boot using _BOOT_ID matches. It starts at the journal head to get the boot ID, makes a _BOOT_ID match and then comes from the opposite journal direction (tail) to get the end a boot. And then flushes the matches, and advances the journal from that exact position one further (which gives us the start and ID of our next boot). Rinse and repeat. Note, v1 differs in that it assumes sd_journal_flush_matches() will also reset the position we are in the journal at that moment. That version went around that by using a cursor and seeking to the after flushing. Hence why I wonder if this behavior of slush_matches is expected/desired or not. This is much faster for relative boot ID lookups, for the very reason that you don't have to look at all boots. Though, it does make the assumption that all boots (IDs) are assumed to not interleave (constellations like A B A C cannot happen), which afaik would be satisfied on single host machines. Later after sending this patch I realized that it could probably break on journals with more than one machine ID, since then boot IDs can interleave due to them running in parallel, breaking a important assumption. Though, I *should* be able to fix that by adding some _MACHINE_ID matches in the mix. Adding machine ID matches would make --list-boots behavior differ quite a lot. For one, with this approach, there isn't any global ordering of boots across machine IDs. Personally, I find this ordering (although you can define it as *a* valid ordering) to be useless. Doing a journalctl -b boodID-1 match, for example, should use that bootID's machine ID to get to the previous boot (of that machine). Right now it can get you any bootID from any other machine, so long as it was booted right before it. So yeah, I will make this patch work for journals with more than one machine ID if this approach is desired. Jan I gave this another look today. Since journalctl uses SD_JOURNAL_LOCAL_ONLY by default, the new algorithm cannot trip up on interleaving boot IDs (since they shouldn't be interleaving in that case, per the above assumption). Same goes for --machine mode. Now, --file, --directory and --merge mode on the other hand does confuse the new algorithm. But I think it might be worth it to go with my above suggestion if that'll be accepted. Alternatively, we could either refuse --boot and --list-boots in those cases, or ship the old algorithm along with the new one and use that one in those cases where the faster one gets confused. Or we stick with status quo and don't improve on the algorithm altogether. I'd like to know the option to go with, to ease me mind... Jan ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v2] journalctl: Improve boot ID lookup
On Thu, 02.04.15 17:08, Jan Janssen (medhe...@web.de) wrote: This method should greatly improve offset based lookup. We now don't have to aggregate the full boot listing just so we can jump to a specific position, which can be a real pain on big journals just for a mere -b -1 case. As an additional benefit --list-boots should improve slightly too, because we now need to do less seeking. Note that there can be a change in boot order with this lookup method because it will use the order of boots in the journal, not the realtime stamp stored in them. That's arguably better, though. https://bugs.freedesktop.org/show_bug.cgi?id=72601 --- Hi, today I realized that it would be nice if we could do without the cursor seeking. Turns out we can! I could swear that I tested sd_journal_flush_matches() would reset our position in the journal. But it seems that sd_journal_next/previous will advance just fine from the last position we were in, even after a flush. Though, I would still like someone with better journal internals knowledge confirm that this is how it's supposed to work. Some testing/timing from others than me would be nice too. Hmm, the patch is hard to read, can you explain what precisely the new algorithm is you propose? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v2] journalctl: Improve boot ID lookup
On 2015-04-08 14:39, Lennart Poettering wrote: On Thu, 02.04.15 17:08, Jan Janssen (medhe...@web.de) wrote: This method should greatly improve offset based lookup. We now don't have to aggregate the full boot listing just so we can jump to a specific position, which can be a real pain on big journals just for a mere -b -1 case. As an additional benefit --list-boots should improve slightly too, because we now need to do less seeking. Note that there can be a change in boot order with this lookup method because it will use the order of boots in the journal, not the realtime stamp stored in them. That's arguably better, though. https://bugs.freedesktop.org/show_bug.cgi?id=72601 --- Hi, today I realized that it would be nice if we could do without the cursor seeking. Turns out we can! I could swear that I tested sd_journal_flush_matches() would reset our position in the journal. But it seems that sd_journal_next/previous will advance just fine from the last position we were in, even after a flush. Though, I would still like someone with better journal internals knowledge confirm that this is how it's supposed to work. Some testing/timing from others than me would be nice too. Hmm, the patch is hard to read, can you explain what precisely the new algorithm is you propose? Lennart Yeah, patches like these always do end up looking messy. It's much easier to read after applying it. Well, it jumps from one boot to the next boot using _BOOT_ID matches. It starts at the journal head to get the boot ID, makes a _BOOT_ID match and then comes from the opposite journal direction (tail) to get the end a boot. And then flushes the matches, and advances the journal from that exact position one further (which gives us the start and ID of our next boot). Rinse and repeat. Note, v1 differs in that it assumes sd_journal_flush_matches() will also reset the position we are in the journal at that moment. That version went around that by using a cursor and seeking to the after flushing. Hence why I wonder if this behavior of slush_matches is expected/desired or not. This is much faster for relative boot ID lookups, for the very reason that you don't have to look at all boots. Though, it does make the assumption that all boots (IDs) are assumed to not interleave (constellations like A B A C cannot happen), which afaik would be satisfied on single host machines. Later after sending this patch I realized that it could probably break on journals with more than one machine ID, since then boot IDs can interleave due to them running in parallel, breaking a important assumption. Though, I *should* be able to fix that by adding some _MACHINE_ID matches in the mix. Adding machine ID matches would make --list-boots behavior differ quite a lot. For one, with this approach, there isn't any global ordering of boots across machine IDs. Personally, I find this ordering (although you can define it as *a* valid ordering) to be useless. Doing a journalctl -b boodID-1 match, for example, should use that bootID's machine ID to get to the previous boot (of that machine). Right now it can get you any bootID from any other machine, so long as it was booted right before it. So yeah, I will make this patch work for journals with more than one machine ID if this approach is desired. Jan ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH v2] journalctl: Improve boot ID lookup
This method should greatly improve offset based lookup. We now don't have
to aggregate the full boot listing just so we can jump to a specific position,
which can be a real pain on big journals just for a mere -b -1 case.
As an additional benefit --list-boots should improve slightly too, because
we now need to do less seeking.
Note that there can be a change in boot order with this lookup method
because it will use the order of boots in the journal, not the realtime stamp
stored in them. That's arguably better, though.
https://bugs.freedesktop.org/show_bug.cgi?id=72601
---
Hi,
today I realized that it would be nice if we could do without the cursor
seeking.
Turns out we can! I could swear that I tested sd_journal_flush_matches() would
reset our position in the journal. But it seems that sd_journal_next/previous
will advance just fine from the last position we were in, even after a flush.
Though, I would still like someone with better journal internals knowledge
confirm
that this is how it's supposed to work.
Some testing/timing from others than me would be nice too.
Jan
src/journal/journalctl.c | 270 +--
1 file changed, 169 insertions(+), 101 deletions(-)
diff --git a/src/journal/journalctl.c b/src/journal/journalctl.c
index b4f88bc..08cd749 100644
--- a/src/journal/journalctl.c
+++ b/src/journal/journalctl.c
@@ -128,6 +128,7 @@ typedef struct boot_id_t {
sd_id128_t id;
uint64_t first;
uint64_t last;
+LIST_FIELDS(struct boot_id_t, boot_list);
} boot_id_t;
static void pager_open_if_enabled(void) {
@@ -851,111 +852,203 @@ static int add_matches(sd_journal *j, char **args) {
return 0;
}
-static int boot_id_cmp(const void *a, const void *b) {
-uint64_t _a, _b;
+static int discover_next_boot(sd_journal *j,
+ boot_id_t **boot,
+ bool advance_older,
+ bool read_realtime) {
+int r;
+char match[9+32+1] = _BOOT_ID=;
+_cleanup_free_ boot_id_t *next_boot = NULL;
-_a = ((const boot_id_t *)a)-first;
-_b = ((const boot_id_t *)b)-first;
+assert(j);
+assert(boot);
-return _a _b ? -1 : (_a _b ? 1 : 0);
-}
+/* We expect the journal to be on the last position of a boot
+ * (in relation to the direction we are going), so that the next
+ * invocation of sd_journal_next/previous will be from a different
+ * boot. We then collect any information we desire and then jump
+ * to the last location of the new boot by using a _BOOT_ID match
+ * coming from the other journal direction. */
-static int get_boots(sd_journal *j,
- boot_id_t **boots,
- unsigned int *count,
- boot_id_t *query_ref_boot) {
-int r;
-const void *data;
-size_t length, allocated = 0;
+/* Make sure we aren't restricted by any _BOOT_ID matches, so that
+ * we can actually advance to a *different* boot. */
+sd_journal_flush_matches(j);
-assert(j);
-assert(boots);
-assert(count);
+if (advance_older)
+r = sd_journal_previous(j);
+else
+r = sd_journal_next(j);
+if (r 0)
+return r;
+else if (r == 0)
+return 0; /* End of journal, yay. */
-r = sd_journal_query_unique(j, _BOOT_ID);
+next_boot = new0(boot_id_t, 1);
+if (!next_boot)
+return log_oom();
+
+r = sd_journal_get_monotonic_usec(j, NULL, next_boot-id);
if (r 0)
return r;
-*count = 0;
-SD_JOURNAL_FOREACH_UNIQUE(j, data, length) {
-boot_id_t *id;
+if (read_realtime) {
+r = sd_journal_get_realtime_usec(j, next_boot-first);
+if (r 0)
+return r;
+}
-assert(startswith(data, _BOOT_ID=));
+/* Now seek to the last occurrence of this boot ID. */
+sd_id128_to_string(next_boot-id, match + 9);
+r = sd_journal_add_match(j, match, sizeof(match) - 1);
+if (r 0)
+return r;
-if (!GREEDY_REALLOC(*boots, allocated, *count + 1))
-return log_oom();
+if (advance_older)
+r = sd_journal_seek_head(j);
+else
+r = sd_journal_seek_tail(j);
+if (r 0)
+return r;
-id = *boots + *count;
+if (advance_older)
+r = sd_journal_next(j);
+else
+r = sd_journal_previous(j);
+if (r 0)
+return r;
+else if (r == 0)
+return -ENODATA; /* This shouldn't happen. We just came from
this very boot ID. */
-r =
