journal: fix _SOURCE_MONOTONIC_TIMESTAMP field#33386
Conversation
The timestamp is not in CLOCK_MONOTONIC, but CLOCK_BOOTTIME, while header monotonic timestamp is in CLOCK_MONOTONIC. Hence, we cannot adjust timestamp by comparing with header monotonic timestamp and _SOURCE_MONOTONIC_TIMESTAMP field. Fixes a regression caused by affde1d. Fixes systemd#33293.
Then, fix the monotonic timestamp. The _SOURCE_MONOTONIC_TIMESTAMP field is already used in other projects. Hence, we cannot remove the field. But, let's store the correct value. The existence of the new _SOURCE_BOOTTIME_TIMESTAMP field can indicate that the monotonic timestamp field is reliable or not.
…STAMP field exists With the previous commit, now the _SOURCE_MONOTONIC_TIMESTAMP field is usable but only when _SOURCE_BOOTTIME_TIMESTAMP exists.
yuwata
added
journal
util-lib
needs-stable-backport
please-review
|
The first commit needs to be backported to v256-stable. |
bluca
added
good-to-merge/waiting-for-ci 👍
github-actions
bot
removed
the
good-to-merge/waiting-for-ci 👍
|
guys, are we in a hurry? there is no need to hurry these things at all? you hurry this stuff into a stable release 12h after it was posted for review? cool down, please. If this was an important regression then maybe this was appropriate, but this is not important nor a regression, so why the hurry? please, let's give non-trivial stuff like this a chance to be reviewed properly. |
|
The ticket says it is a regression (it's marked as such), and logs are being lost, which to me sounds kinda important and worth including in the stable release. And it was open the whole working day, and sitting with a green label for like 5 hours, so it's not like it all happened within 5 minutes... |
|
Actually the last two commits do not work... |
src/shared/logs-show.c
Outdated
| assert(ret); | ||
|
|
||
| // FIXME: _SOURCE_MONOTONIC_TIMESTAMP is in CLOCK_BOOTTIME, hence we cannot use it for adjusting realtime. | ||
| source_monotonic = NULL; |
There was a problem hiding this comment.
hmm, i am not sure i grok this comment. i mean we shouldn't really fix this up when parsing, but simply fill in the correct fields.
note there has been a long standing issue that we probably should include the CLOCK_BOOTTIME timestamp in all journal entries too (right now we only store the realtime + monotonic timestamp). There's even a bug open about that somewhere I think, was unable to find it quickly though.
I couldn't come up with a nice way to include the boottime timestamp in the records without massively breaking the format and increasing the entry record size so far. The least bad idea i had, is that depending on some feature flag we'd use the upper 32bit of seqnum, plus the upper 8bit of the monotonic + realtime fields of EntryObject to encode the offset of boottime towards seqnum. That would give us 32+8+8 = 48bit accuracy, i.e we'd allow 9 years or so of delta between CLOCK_MONOTONIC and CLOCK_BOOTTIME. i.e. people could hibernate their system 9 year long before the journal couldn't deal with it anymore.
There was a problem hiding this comment.
Yeah, I am considering something similar now. Saving shift is slightly complicated, but can keep entry object size, hence maybe good idea.
|
maybe a "compatible" header field should indicate whether _SOURCE_MONOTONIC_TIMESTAMP actually means what it says on the tin, or is actually _SOURCE_BOOTTIME_TIMESTAMP. i.e. HEADER_COMPATIBLE_SOURCE_MONOTONIC_WORKS or so as new flag, next to the other HEADER_COMPATIBLE_* flags. |
As commented in #33401 (comment), I have no idea to make _SOURCE_MONOTONIC_TIMESTAMP reliable. If there is a way, then may be better to introduce a flag, of course. |
3 participants
Fixes #33293.