And here is another one, as was discussed where we replace a static array with a mmap allocation in the lifeboat.
Regards, Milos On Mon, Aug 31, 2026 at 5:04 PM Milos Nikic <[email protected]> wrote: > 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 2ccb5452d7e492e435c1df51958a41ee6f97d21e Mon Sep 17 00:00:00 2001 From: Milos Nikic <[email protected]> Date: Mon, 31 Aug 2026 08:36:44 -0700 Subject: [PATCH] ext2fs: dynamically allocate ext2_lifeboat payloads via mmap Previously, the ext2_lifeboat payloads array was statically sized with a hardcoded 4096-byte assumption, which breaks on filesystems formatted with different block sizes. Changing the payload to a pointer and allocating it dynamically via mmap() during journal creation solves two issues: 1. It scales correctly with the actual block_size of the filesystem. 2. The virtual memory manager inherently provides page-aligned memory, which should help with performance. --- ext2fs/journal.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/ext2fs/journal.c b/ext2fs/journal.c index 91b8e64ff..b25bf9de7 100644 --- a/ext2fs/journal.c +++ b/ext2fs/journal.c @@ -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; }; static struct journal_lifeboat ext2_lifeboat; @@ -1114,8 +1114,8 @@ journal_stop_transaction_locked (journal_t *journal, if (jb_exp->lifeboat_index >= 0) { memcpy (jb_exp->jb_shadow_data, - ext2_lifeboat.payloads[jb_exp->lifeboat_index], - block_size); + &(ext2_lifeboat.payloads)[jb_exp->lifeboat_index * + block_size], block_size); jb_exp->needs_copy = 0; } else @@ -1378,6 +1378,11 @@ journal_create (struct node *journal_inode) j->j_pool_memory[JRNL_MAX_FREE_BUFFERS - 1].jb_next = NULL; j->j_free_buffers = &j->j_pool_memory[0]; + ext2_lifeboat.payloads = + mmap (NULL, JRNL_LIFEBOAT_CAPACITY * block_size, PROT_READ | PROT_WRITE, + MAP_ANON | MAP_PRIVATE, -1, 0); + if (ext2_lifeboat.payloads == MAP_FAILED) + ext2_panic ("[JOURNAL] No RAM for lifeboat cache!"); return j; } @@ -1862,7 +1867,7 @@ journal_flush_lifeboat_payloads (journal_t *journal, /* We do the I/O using our safely captured, privately owned index */ err = store_write (store, dev_block, - ext2_lifeboat.payloads[lb_idx], + &(ext2_lifeboat.payloads)[lb_idx * block_size], block_size, &amount); JOURNAL_LOCK (journal); @@ -2217,15 +2222,16 @@ journal_handle_write_hazard_locked (block_t b, char *b_data) /* Success: Spoof the write directly into the Lifeboat */ if (jb_run) { - memcpy (ext2_lifeboat.payloads[lb_idx_run], b_data, block_size); + memcpy (&(ext2_lifeboat.payloads)[lb_idx_run * block_size], + b_data, block_size); if (jb_run->lifeboat_index >= 0) lifeboat_free_slot (jb_run->lifeboat_index); jb_run->lifeboat_index = (int16_t) lb_idx_run; } if (jb_commit) { - memcpy (ext2_lifeboat.payloads[lb_idx_commit], b_data, - block_size); + memcpy (&(ext2_lifeboat.payloads)[lb_idx_commit * block_size], + b_data, block_size); /* If the old slot is NOT being flushed, we must free it to avoid a leak. If it IS being flushed, the commit thread owns it and will free it. */ if (jb_commit->lifeboat_index >= 0 @@ -2421,7 +2427,8 @@ journal_overlay_lifeboat (block_t start_block, size_t length, void *buf) /* Overlay the fresh RAM data safely! */ memcpy (out_ptr + offset, - ext2_lifeboat.payloads[jb->lifeboat_index], copy_len); + &(ext2_lifeboat.payloads)[jb->lifeboat_index * block_size], + copy_len); JRNL_LOG_DEBUG ("Lifeboat Overlay successful for block %u (copied %zu bytes)", b, -- 2.55.0
