On Fri 05-09-25 11:25:49, Sun Yongjian wrote:
> 在 2025/9/4 17:11, Jan Kara 写道:
> > 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. Analysis revealed the
> > > > following call chain that leads to the inconsistency:
> > > >
> > > > ext4_da_write_end()
> > > > //does not update i_disksize
> > > > ext4_punch_hole()
> > > > //truncate folio, keep size
> > > > ext4_page_mkwrite()
> > > > ext4_block_page_mkwrite()
> > > > ext4_block_write_begin()
> > > > ext4_get_block()
> > > > //insert written extent without update i_disksize
> > > > 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>
> >
> > OK, after the discussion with Ritesh your solution looks like the best one.
> > Just two nits below:
> >
> > > > ---
> > > > 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;
> >
> > Use an empty line between declarations and the code please.
> >
> > > > 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;
> > > > + }
> >
> > Since we don't support delalloc with data journalling, your code is correct
> > but I think it would be more understandable if you just moved the
> > i_disksize update outside of the "if (!ext4_should_journal_data(inode))"
> > condition.
> >
> > > > } else {
> > > > ret = ext4_journal_folio_buffers(handle, folio, len);
> > > > if (ret)
> > >
> >
> > Honza
> Thanks for the review, I will send a patch to improve this!
Yesterday on ext4 developers call we were further discussing this and Ted
came up with a different way of addressing this issue which might be even
better. Instead of updating i_disksize in ext4_page_mkwrite() we can
instead update i_disksize already during the hole punch. I.e., we can modify
ext4_update_disksize_before_punch() to always increase i_disksize to offset
+ len. That should deal with the problem as well and we would avoid
updating i_disksize from page_mkwrite() which is a bit awkward special case.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
