Applied, thanks! Milos Nikic, le lun. 31 août 2026 20:32:39 -0700, a ecrit: > 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 <[1][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 <[2] > [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 > > > References: > > [1] mailto:[email protected] > [2] mailto:[email protected]
> 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 > -- Samuel "c'est pas nous qui sommes à la rue, c'est la rue qui est à nous"
