Ok thanks for that. Also thanks for your specific qemu command, i managed today to run it with a separate disk and the driver and manage to reproduce the inconsistency bug. (but not yet corruption, and a deadlock).
For the "needs_recovery flag is clear, but journal has data" the problem is the two flags were kind of coupled in ext2 as it is. Since we now have journal they have to be decoupled to be able to express all the situations. This patch decouples them so that i don't get that "needs_recovery flag is clear, but journal has data" problem. Take a look please. I will keep digging and trying to work on the other observed issues. Kind regards, Milos On Mon, Aug 31, 2026 at 11:40 AM Samuel Thibault <[email protected]> wrote: > Hello, > > Milos Nikic, le lun. 31 août 2026 08:54:23 -0700, a ecrit: > > But from what I can tell, it seems that memory misalignment in the > > ext2_lifeboat cache is the root of the problem. > > What memory misalignment? > > > @@ -162,7 +162,7 @@ struct journal_lifeboat > > uint64_t alloc_mask[JRNL_LIFEBOAT_ALLOC_MASK_LEN]; > > > > /* The pre-allocated payload pool (512 * 4KB = 2MB) */ > > - char payloads[JRNL_LIFEBOAT_CAPACITY][4096]; > > + char payloads[JRNL_LIFEBOAT_CAPACITY][4096] > __attribute__((aligned(4096))); > > }; > > > > static struct journal_lifeboat ext2_lifeboat; > > I don't see the benefit of this? It is only memcpy'd or passed to > store_write, which cope with unaligned data fine. > > Aligning would help with performance, though. > > That actually makes me realize: this 4096 here is hardcoded, and > block_size is assumed to be that. > > Better make payloads a char*, and allocate the whole payloads array > dynamically with mmap(JRNL_LIFEBOAT_CAPACITY*block_size) and access it > with &payloads[i*block_size] > > > This small patch fixes the alignment and also adds > > As always in all software projects, please keep unrelated changes > separate, so they can be tested independently easily. > > > a physical hardware flush to the end of the journal shutdown. > > > diff --git a/ext2fs/journal.c b/ext2fs/journal.c > > index 91b8e64ff..8b03604ab 100644 > > --- a/ext2fs/journal.c > > +++ b/ext2fs/journal.c > > @@ -1459,6 +1459,7 @@ journal_quiesce_checkpoints (void) > > /* Clear the list and write s_start = 0 to the JBD2 superblock */ > > journal_clear_checkpoint_list_locked (ext2_journal); > > JOURNAL_UNLOCK (ext2_journal); > > + flush_to_disk (); > > } > > I don't see why adding it here: it's only at filesystem shutdown that > we want to make sure that the updates hit the disk. Put another way, > it's diskfs_shutdown_pager that we want to see flush things, and that > already calls store_sync, so there is some problem somewhere along the > path, to be just fixed rather than add flushing calls that would mostly > brown-tape-fix with performance impact. > > With regards, > Samuel >
From fad85714ceb7e876df9ff9b949f96e237dea1cdf Mon Sep 17 00:00:00 2001 From: Milos Nikic <[email protected]> Date: Mon, 31 Aug 2026 16:18:42 -0700 Subject: [PATCH] ext2fs: Decouple EXT3_FEATURE_INCOMPAT_RECOVER from EXT2_VALID_FS This was leading to skipped journal replay in some cases even when journal correctly contained information about what needs correcting. --- ext2fs/ext2fs.c | 7 ++++- ext2fs/hyper.c | 68 ++++++++++++++++++++++++++++--------------------- 2 files changed, 45 insertions(+), 30 deletions(-) diff --git a/ext2fs/ext2fs.c b/ext2fs/ext2fs.c index 984df0448..469a7f132 100644 --- a/ext2fs/ext2fs.c +++ b/ext2fs/ext2fs.c @@ -268,7 +268,12 @@ main (int argc, char **argv) { ext2_journal = journal_create (jnode); if (ext2_journal) - fprintf (stderr, "ext2fs: journaling enabled on %s\n", diskfs_disk_name); + { + fprintf (stderr, "ext2fs: journaling enabled on %s\n", diskfs_disk_name); + /* Immediately mark the filesystem dirty and set needs_recovery on disk */ + if (!diskfs_readonly) + diskfs_set_hypermetadata (1, 0); + } JRNL_LOG_DEBUG ("Global Journal Initialized at %p", ext2_journal); diskfs_nput(jnode); } diff --git a/ext2fs/hyper.c b/ext2fs/hyper.c index a923591e0..6d6fe7d71 100644 --- a/ext2fs/hyper.c +++ b/ext2fs/hyper.c @@ -193,41 +193,51 @@ map_hypermetadata (void) error_t diskfs_set_hypermetadata (int wait, int clean) { - if (clean && ext2fs_clean && !(sblock->s_state & htole16 (EXT2_VALID_FS))) - /* The filesystem is clean, so we need to set the clean flag. */ + if (clean) { - sblock->s_state |= htole16 (EXT2_VALID_FS); - if (ext2_journal) - { - sblock->s_feature_incompat &= htole32(~EXT3_FEATURE_INCOMPAT_RECOVER); - } - sblock_dirty = 1; + /* Always clear recovery flag on clean unmount if journal is present */ + if (ext2_journal && (sblock->s_feature_incompat & htole32(EXT3_FEATURE_INCOMPAT_RECOVER))) + { + sblock->s_feature_incompat &= htole32(~EXT3_FEATURE_INCOMPAT_RECOVER); + sblock_dirty = 1; + } + /* Only set EXT2_VALID_FS if it was clean when we mounted it */ + if (ext2fs_clean && !(sblock->s_state & htole16 (EXT2_VALID_FS))) + { + sblock->s_state |= htole16 (EXT2_VALID_FS); + sblock_dirty = 1; + } } - else if (!clean && (sblock->s_state & htole16 (EXT2_VALID_FS))) - /* The filesystem just became dirty, so clear the clean flag. */ + else { - if (ext2_journal && - !(sblock->s_feature_incompat & htole32(EXT3_FEATURE_INCOMPAT_RECOVER))) - { - sblock->s_feature_incompat |= htole32(EXT3_FEATURE_INCOMPAT_RECOVER); + /* Always set recovery flag when dirtying if journal is present */ + if (ext2_journal && !(sblock->s_feature_incompat & htole32(EXT3_FEATURE_INCOMPAT_RECOVER))) + { + sblock->s_feature_incompat |= htole32(EXT3_FEATURE_INCOMPAT_RECOVER); + sblock_dirty = 1; + wait = 1; + } + /* Clear EXT2_VALID_FS */ + if (sblock->s_state & htole16 (EXT2_VALID_FS)) + { + sblock->s_state &= htole16 (~EXT2_VALID_FS); + sblock_dirty = 1; + wait = 1; } - sblock->s_state &= htole16 (~EXT2_VALID_FS); - sblock_dirty = 1; - wait = 1; } - if (sblock_dirty) - { - if (diskfs_readonly) - return EROFS; /* impossible to write */ - - /* Before writing, set the time of write */ - sblock->s_wtime = htole32 (diskfs_mtime->seconds); - sblock_dirty = 0; - memcpy (mapped_sblock, sblock, SBLOCK_SIZE); - disk_cache_block_ref_ptr (mapped_sblock); - record_global_poke (mapped_sblock); - } + if (sblock_dirty) + { + if (diskfs_readonly) + return EROFS; /* impossible to write */ + + /* Before writing, set the time of write */ + sblock->s_wtime = htole32 (diskfs_mtime->seconds); + sblock_dirty = 0; + memcpy (mapped_sblock, sblock, SBLOCK_SIZE); + disk_cache_block_ref_ptr (mapped_sblock); + record_global_poke (mapped_sblock); + } sync_global (wait); if (wait) -- 2.55.0
