Jan Kara <jack@xxxxxxx> writes: > On Mon 01-09-25 15:01:45, Sun Yongjian wrote: >> 在 2025/7/31 22:05, sunyongjian@xxxxxxxxxxxxxxx 写道: >> Gentle ping. >> > From: Yongjian Sun <sunyongjian1@xxxxxxxxxx> >> > >> > After running a stress test combined with fault injection, >> > we performed fsck -a followed by fsck -fn on the filesystem >> > image. During the second pass, fsck -fn reported: >> > >> > Inode 131512, end of extent exceeds allowed value >> > (logical block 405, physical block 1180540, len 2) >> > >> > This inode was not in the orphan list. > > Thanks for report! Interesting... Which kernel were you using? > >> > Analysis revealed the >> > following call chain that leads to the inconsistency: >> > >> > ext4_da_write_end() >> > //does not update i_disksize > > Right, for any write beyond i_disksize to unallocated blocks we update > i_disksize only during page writeback. > >> > ext4_punch_hole() >> > //truncate folio, keep size > > So here offset + len passed to ext4_punch_hole() is important. Because > there's ext4_update_disksize_before_punch() call which updates i_disksize > to i_size if the punched hole reaches EOF. So did you punch hole in the > middle of the file? > >> > ext4_page_mkwrite() >> > ext4_block_page_mkwrite() >> > ext4_block_write_begin() >> > ext4_get_block() >> > //insert written extent without update i_disksize > > We should insert unwritten extent here, shouldn't we? We use > ext4_get_block_unwritten() when we are inside i_size. Ah, you mention below > you use nodioread_nolock. Nasty :) > >> > journal commit >> > echo 1 > /sys/block/xxx/device/delete >> > >> > da-write path updates i_size but does not update i_disksize. Then >> > ext4_punch_hole truncates the da-folio yet still leaves i_disksize >> > unchanged. Then ext4_page_mkwrite sees ext4_nonda_switch return 1 >> > and takes the nodioread_nolock path, the folio about to be written >> > has just been punched out, and it’s offset sits beyond the current >> > i_disksize. This may result in a written extent being inserted, but >> > again does not update i_disksize. If the journal gets committed and >> > then the block device is yanked, we might run into this. >> > >> > To fix this, we now check in ext4_block_page_mkwrite whether >> > i_disksize needs to be updated to cover the newly allocated blocks. >> > >> > Signed-off-by: Yongjian Sun <sunyongjian1@xxxxxxxxxx> > > Hum, rather than complicating this niche code what if we just > unconditionally used ext4_get_block_unwritten() in > ext4_block_page_mkwrite() when delalloc gets disabled? It is far from any > performance critical path. What do people think? The code would actually > have to be something like: > > if (ext4_should_journal_data(inode)) > get_block = ext4_get_block; > else > get_block = ext4_get_block_unwritten; > The problem mainly was when e2fsck identify a written extent beyond i_disksize. But if it is unwritten extent, then we are still good. So using ext4_get_block_unwritten() by default in page fault path make sense. So what about other checks like S_ISREG() and file with indirect blocks in ext4_should_dioread_nolock()? We still need ext4_get_block() for them right? (Do we even fault on !S_ISREG() :) ?) > to properly handle data journalling. I'm adding Ritesh to CC because I do > remember there used to be some issues with dioread_nolock with blocksize < > pagesize which he was able to trigger. But I think they were fixed. > Right, they were fixed and dioread_nolock is now the default mount option for ext4 for bs <= ps. Here are some of the fixes which were made. [2] was the more recent one from Ojaswin. [1]: https://lore.kernel.org/all/af902b5db99e8b73980c795d84ad7bb417487e76.1602168865.git.riteshh@xxxxxxxxxxxxx/ [2]: https://lore.kernel.org/all/d0ed09d70a9733fbb5349c5c7b125caac186ecdf.1695033645.git.ojaswin@xxxxxxxxxxxxx/ [3]: This patch enabled dioread_nolock for bs < ps.. https://lore.kernel.org/all/20231101154717.531865-1-ojaswin@xxxxxxxxxxxxx/ > Honza > >> > --- >> > fs/ext4/inode.c | 10 ++++++++++ >> > 1 file changed, 10 insertions(+) >> > >> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >> > index ed54c4d0f2f9..050270b265ae 100644 >> > --- a/fs/ext4/inode.c >> > +++ b/fs/ext4/inode.c >> > @@ -6666,8 +6666,18 @@ static int ext4_block_page_mkwrite(struct inode *inode, struct folio *folio, >> > goto out_error; >> > if (!ext4_should_journal_data(inode)) { >> > + loff_t disksize = folio_pos(folio) + len; >> > block_commit_write(folio, 0, len); >> > folio_mark_dirty(folio); >> > + if (disksize > READ_ONCE(EXT4_I(inode)->i_disksize)) { >> > + down_write(&EXT4_I(inode)->i_data_sem); >> > + if (disksize > EXT4_I(inode)->i_disksize) >> > + EXT4_I(inode)->i_disksize = disksize; >> > + up_write(&EXT4_I(inode)->i_data_sem); >> > + ret = ext4_mark_inode_dirty(handle, inode); >> > + if (ret) >> > + goto out_error; >> > + } >> > } else { >> > ret = ext4_journal_folio_buffers(handle, folio, len); >> > if (ret) >> > -- > Jan Kara <jack@xxxxxxxx> > SUSE Labs, CR
