journal: recover iteration over vacuumed files (#43649)

Vacuuming unlinks and deallocates archived journal files. A reader may have already selected an entry from such a file, so final validation can fail after candidate selection succeeded. If the selected file is now unlinked, remove it and retry candidate selection. Preserve errors from linked files, including the EIO reported after SIGBUS recovery from a truncated mapping.
2026-09-05 11:17:59 +09:00
parent d57ad97128
commit 101b8633c2
2 changed files with 186 additions and 47 deletions
@@ -1127,69 +1127,88 @@ static int compare_locations(sd_journal *j, JournalFile *af, JournalFile *bf) {
 }

 static int real_journal_next(sd_journal *j, direction_t direction) {
-        JournalFile *new_file = NULL, *exact_match = NULL;
        unsigned n_files;
        const void **files;
-        Object *o;
        int r;

        assert_return(j, -EINVAL);
        assert_return(!journal_origin_changed(j), -ECHILD);

-        r = iterated_cache_get(j->files_cache, NULL, &files, &n_files);
-        if (r < 0)
-                return r;
+        for (;;) {
+                JournalFile *new_file = NULL, *exact_match = NULL;
+                Object *o;

-        FOREACH_ARRAY(_f, files, n_files) {
-                JournalFile *f = (JournalFile*) *_f;
-                bool found;
+                r = iterated_cache_get(j->files_cache, NULL, &files, &n_files);
+                if (r < 0)
+                        return r;

-                r = next_beyond_location(j, f, direction);
-                if (r < 0) {
-                        log_debug_errno(r, "Can't iterate through %s, ignoring: %m", f->path);
-                        remove_file_real(j, f);
-                        continue;
-                } else if (r == 0) {
-                        f->location_type = direction == DIRECTION_DOWN ? LOCATION_TAIL : LOCATION_HEAD;
-                        continue;
+                FOREACH_ARRAY(_f, files, n_files) {
+                        JournalFile *f = (JournalFile*) *_f;
+                        bool found;
+
+                        r = next_beyond_location(j, f, direction);
+                        if (r < 0) {
+                                log_debug_errno(r, "Can't iterate through %s, ignoring: %m", f->path);
+                                remove_file_real(j, f);
+                                continue;
+                        } else if (r == 0) {
+                                f->location_type = direction == DIRECTION_DOWN ?
+                                        LOCATION_TAIL : LOCATION_HEAD;
+                                continue;
+                        }
+
+                        if (!new_file)
+                                found = true;
+                        else {
+                                r = compare_locations(j, f, new_file);
+                                found = direction == DIRECTION_DOWN ? r < 0 : r > 0;
+                        }
+
+                        if (found)
+                                new_file = f;
+
+                        /* Track the file that holds the cursor's exact entry (matching seqnum_id and
+                         * seqnum). On systems without a reliable (or missing) RTC, compare_boot_ids() can
+                         * produce incorrect cross-boot ordering. After the loop, we detect when
+                         * compare_locations() preferred the wrong file and override the choice if needed.
+                         *
+                         * See https://github.com/systemd/systemd/issues/31516 */
+                        if (j->current_location.type == LOCATION_SEEK &&
+                            j->current_location.seqnum_set &&
+                            sd_id128_equal(f->header->seqnum_id, j->current_location.seqnum_id) &&
+                            f->current_seqnum == j->current_location.seqnum)
+                                exact_match = f;
                }

+                if (exact_match)
+                        new_file = exact_match;
+
                if (!new_file)
-                        found = true;
-                else {
-                        r = compare_locations(j, f, new_file);
-                        found = direction == DIRECTION_DOWN ? r < 0 : r > 0;
+                        return 0;
+
+                r = journal_file_move_to_object(new_file, OBJECT_ENTRY, new_file->current_offset, &o);
+                if (r < 0) {
+                        /* Vacuuming can unlink and deallocate the file we just picked, which surfaces as
+                         * one of these errors. Confirm the file is really gone before dropping it: -EIDRM
+                         * already comes from the journal_file_fstat() inside the lookup, the others need a
+                         * stat of our own. */
+                        if (!IN_SET(r, -EADDRNOTAVAIL, -EBADMSG, -EIDRM, -EIO))
+                                return r;
+                        if (r != -EIDRM && journal_file_fstat(new_file) != -EIDRM)
+                                return r;
+
+                        log_debug_errno(r, "Can't read selected entry from removed journal file '%s', ignoring: %m",
+                                        new_file->path);
+                        remove_file_real(j, new_file);
+
+                        /* Removing the selected file guarantees that the next iteration makes progress. */
+                        continue;
                }

-                if (found)
-                        new_file = f;
+                set_location(j, new_file, o);

-                /* Track the file that holds the cursor's exact entry (matching seqnum_id and seqnum). On
-                 * systems without a reliable (or missing) RTC, compare_boot_ids() can produce incorrect
-                 * cross-boot ordering causing compare_locations() above to prefer a wrong file. We detect
-                 * this after the loop and override the choice if needed.
-                 *
-                 * See https://github.com/systemd/systemd/issues/31516 */
-                if (j->current_location.type == LOCATION_SEEK &&
-                    j->current_location.seqnum_set &&
-                    sd_id128_equal(f->header->seqnum_id, j->current_location.seqnum_id) &&
-                    f->current_seqnum == j->current_location.seqnum)
-                        exact_match = f;
+                return 1;
        }
-
-        if (exact_match)
-                new_file = exact_match;
-
-        if (!new_file)
-                return 0;
-
-        r = journal_file_move_to_object(new_file, OBJECT_ENTRY, new_file->current_offset, &o);
-        if (r < 0)
-                return r;
-
-        set_location(j, new_file, o);
-
-        return 1;
 }

 _public_ int sd_journal_next(sd_journal *j) {
@@ -3,20 +3,28 @@
 #include <fcntl.h>
 #include <unistd.h>

+#if HAVE_VALGRIND_VALGRIND_H
+#  include <valgrind/valgrind.h>
+#endif
+
 #include "sd-id128.h"
 #include "sd-journal.h"

 #include "alloc-util.h"
 #include "argv-util.h"
 #include "chattr-util.h"
+#include "fd-util.h"
+#include "hashmap.h"
 #include "iovec-util.h"
 #include "journal-file-util.h"
+#include "journal-internal.h"
 #include "journal-vacuum.h"
 #include "log.h"
 #include "logs-show.h"
 #include "parse-util.h"
 #include "random-util.h"
 #include "rm-rf.h"
+#include "sigbus.h"
 #include "strv.h"
 #include "tests.h"
 #include "time-util.h"
@@ -447,6 +455,118 @@ TEST(skip) {
        test_skip_one(setup_interleaved);
 }

+/* Read the first entry so that two.journal becomes the next candidate with LOCATION_SEEK, then simulate
+ * vacuuming removing it while it is still open and mapped. */
+static void test_remove_unlinked_selected_file_one(bool truncate, bool refresh_stat) {
+        _cleanup_(test_donep) char *t = NULL;
+        _cleanup_(sd_journal_closep) sd_journal *j = NULL;
+        _cleanup_close_ int fd = -EBADF;
+        uint8_t type = OBJECT_UNUSED;
+        JournalFile *f;
+        struct stat st;
+
+        ASSERT_TRUE(!refresh_stat || truncate);
+
+        mkdtemp_chdir_chattr("/var/tmp/journal-unlinked-XXXXXX", &t);
+        setup_interleaved();
+
+        ASSERT_OK(sd_journal_open_directory(&j, t, SD_JOURNAL_ASSUME_IMMUTABLE));
+        ASSERT_OK(sd_journal_seek_head(j));
+        ASSERT_OK_POSITIVE(sd_journal_next(j));
+        test_check_number(j, 1);
+
+        f = ASSERT_NOT_NULL(ordered_hashmap_get(j->files, strjoina(t, "/two.journal")));
+        ASSERT_EQ((int) f->location_type, LOCATION_SEEK);
+
+        if (truncate && !refresh_stat)
+                sigbus_install();
+
+        /* Keep the file open and mapped while the name goes away, like vacuuming does. */
+        fd = ASSERT_OK_ERRNO(open("two.journal", O_WRONLY|O_CLOEXEC));
+        ASSERT_OK_ERRNO(unlink("two.journal"));
+
+        if (truncate) {
+                /* The lookup only stats the file when the selected entry falls outside the cached size, and
+                 * journal_file_fstat() then reports the unlinked file as -EIDRM. Truncating to the selected
+                 * offset and refreshing the stat takes that path. Truncating the whole file without
+                 * refreshing leaves the cached size stale, so touching the mapping raises SIGBUS and the
+                 * lookup fails with -EIO. */
+                ASSERT_OK_ERRNO(ftruncate(fd, refresh_stat ? f->current_offset : 0));
+                if (refresh_stat)
+                        ASSERT_ERROR(journal_file_fstat(f), EIDRM);
+                else {
+                        ASSERT_EQ(READ_NOW(f->header->state), STATE_OFFLINE);
+                        ASSERT_TRUE(mmap_cache_fd_got_sigbus(f->cache_fd));
+                }
+        } else {
+                /* Emulate a deallocated range, which reads back as zeroes, by zeroing the selected entry's
+                 * object type. */
+                ASSERT_OK_EQ_ERRNO(pwrite(fd, &type, sizeof(type), f->current_offset), (ssize_t) sizeof(type));
+                ASSERT_OK_ERRNO(fsync(fd));
+        }
+
+        ASSERT_OK_ERRNO(fstat(f->fd, &st));
+        ASSERT_EQ(st.st_nlink, 0U);
+        if (refresh_stat)
+                ASSERT_EQ((uint64_t) st.st_size, f->current_offset);
+        else if (truncate)
+                ASSERT_EQ(st.st_size, 0);
+
+        ASSERT_OK_POSITIVE(sd_journal_next(j));
+        if (truncate && !refresh_stat)
+                sigbus_reset();
+        test_check_number(j, 3);
+        ASSERT_NULL(ordered_hashmap_get(j->files, strjoina(t, "/two.journal")));
+}
+
+TEST(remove_unlinked_selected_file) {
+        test_remove_unlinked_selected_file_one(/* truncate= */ false, /* refresh_stat= */ false);
+}
+
+TEST(remove_truncated_unlinked_selected_file) {
+#if HAS_FEATURE_ADDRESS_SANITIZER
+        return (void) log_tests_skipped("SIGBUS recovery cannot be tested under AddressSanitizer");
+#endif
+#if HAVE_VALGRIND_VALGRIND_H
+        if (RUNNING_ON_VALGRIND)
+                return (void) log_tests_skipped("SIGBUS recovery cannot be tested under Valgrind");
+#endif
+
+        test_remove_unlinked_selected_file_one(/* truncate= */ true, /* refresh_stat= */ false);
+}
+
+TEST(remove_truncated_unlinked_selected_file_after_fstat) {
+        test_remove_unlinked_selected_file_one(/* truncate= */ true, /* refresh_stat= */ true);
+}
+
+/* Corruption in a file that is still linked must stay visible to the caller. Whether libsystemd should
+ * skip such a file, or just the corrupt entry, is a separate question; this only pins today's behaviour. */
+TEST(keep_linked_selected_file_error) {
+        _cleanup_(test_donep) char *t = NULL;
+        _cleanup_(sd_journal_closep) sd_journal *j = NULL;
+        _cleanup_close_ int fd = -EBADF;
+        JournalFile *f;
+        uint8_t type = OBJECT_UNUSED;
+
+        mkdtemp_chdir_chattr("/var/tmp/journal-linked-XXXXXX", &t);
+        setup_interleaved();
+
+        ASSERT_OK(sd_journal_open_directory(&j, t, SD_JOURNAL_ASSUME_IMMUTABLE));
+        ASSERT_OK(sd_journal_seek_head(j));
+        ASSERT_OK_POSITIVE(sd_journal_next(j));
+        test_check_number(j, 1);
+
+        f = ASSERT_NOT_NULL(ordered_hashmap_get(j->files, strjoina(t, "/two.journal")));
+        ASSERT_EQ((int) f->location_type, LOCATION_SEEK);
+
+        fd = ASSERT_OK_ERRNO(open("two.journal", O_WRONLY|O_CLOEXEC));
+        ASSERT_OK_EQ_ERRNO(pwrite(fd, &type, sizeof(type), f->current_offset), (ssize_t) sizeof(type));
+        ASSERT_OK_ERRNO(fsync(fd));
+
+        ASSERT_ERROR(sd_journal_next(j), EBADMSG);
+        ASSERT_NOT_NULL(ordered_hashmap_get(j->files, strjoina(t, "/two.journal")));
+}
+
 static void test_boot_id_one(void (*setup)(void), size_t n_ids_expected) {
        _cleanup_(test_donep) char *t = NULL;
        _cleanup_(sd_journal_closep) sd_journal *j = NULL;