[PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[sunyongjian]

From: Yongjian Sun <sunyongjian1@huawei.com>

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@huawei.com>
---
 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)
-- 
2.39.2

Re: [PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[Sun Yongjian]

在 2025/7/31 22:05, sunyongjian@huaweicloud.com 写道:
Gentle ping.
> From: Yongjian Sun <sunyongjian1@huawei.com>
> 
> 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@huawei.com>
> ---
>   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)

Re: [PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[Jan Kara]

On Mon 01-09-25 15:01:45, Sun Yongjian wrote:
> 在 2025/7/31 22:05, sunyongjian@huaweicloud.com 写道:
> Gentle ping.
> > From: Yongjian Sun <sunyongjian1@huawei.com>
> > 
> > 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@huawei.com>

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;

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.

								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@suse.com>
SUSE Labs, CR

Re: [PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[Ritesh Harjani]

Jan Kara <jack@suse.cz> writes:

> On Mon 01-09-25 15:01:45, Sun Yongjian wrote:
>> 在 2025/7/31 22:05, sunyongjian@huaweicloud.com 写道:
>> Gentle ping.
>> > From: Yongjian Sun <sunyongjian1@huawei.com>
>> > 
>> > 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@huawei.com>
>
> 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@linux.ibm.com/
[2]: https://lore.kernel.org/all/d0ed09d70a9733fbb5349c5c7b125caac186ecdf.1695033645.git.ojaswin@linux.ibm.com/
[3]: This patch enabled dioread_nolock for bs < ps.. 
https://lore.kernel.org/all/20231101154717.531865-1-ojaswin@linux.ibm.com/

> 								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@suse.com>
> SUSE Labs, CR

Re: [PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[Jan Kara]

On Thu 04-09-25 09:33:56, Ritesh Harjani wrote:
> Jan Kara <jack@suse.cz> writes:
> > On Mon 01-09-25 15:01:45, Sun Yongjian wrote:
> >> 在 2025/7/31 22:05, sunyongjian@huaweicloud.com 写道:
> >> Gentle ping.
> >> > From: Yongjian Sun <sunyongjian1@huawei.com>
> >> > 
> >> > 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@huawei.com>
> >
> > 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() :) ?)

S_ISREG() is always true for page faults but I forgot about indirect block
based inodes, there we indeed cannot create unwritten extent. Drat, so
probably what Sun suggests is the easiest solution in the end. Going back
to his patch.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[Jan Kara]

On Mon 01-09-25 15:01:45, Sun Yongjian wrote:
> 在 2025/7/31 22:05, sunyongjian@huaweicloud.com 写道:
> Gentle ping.
> > From: Yongjian Sun <sunyongjian1@huawei.com>
> > 
> > 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@huawei.com>

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[Sun Yongjian]

在 2025/9/4 17:11, Jan Kara 写道:
> On Mon 01-09-25 15:01:45, Sun Yongjian wrote:
>> 在 2025/7/31 22:05, sunyongjian@huaweicloud.com 写道:
>> Gentle ping.
>>> From: Yongjian Sun <sunyongjian1@huawei.com>
>>>
>>> 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@huawei.com>
> 
> 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!

Re: [PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[Jan Kara]

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@huaweicloud.com 写道:
> > > Gentle ping.
> > > > From: Yongjian Sun <sunyongjian1@huawei.com>
> > > > 
> > > > 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@huawei.com>
> > 
> > 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@suse.com>
SUSE Labs, CR

Re: [PATCH -next] ext4: add an update to i_disksize in ext4_block_page_mkwrite

[Sun Yongjian]

在 2025/9/5 20:58, Jan Kara 写道:
> 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@huaweicloud.com 写道:
>>>> Gentle ping.
>>>>> From: Yongjian Sun <sunyongjian1@huawei.com>
>>>>>
>>>>> 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@huawei.com>
>>>
>>> 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
> 
I believe this bring a more elegant approach to the matter, let's try this!