Re: [PATCH v6 18/31] ext4: drain writeback before removing extents on the iomap path

[Ojaswin Mujoo]

On Thu, Sep 03, 2026 at 08:35:30PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> Because the iomap infrastructure does not always create an ifs to
> manage sub-folio state when folio size is larger than blocksize,
> invalidating a partial dirty folio during punch hole may fail to
> clear the dirty state of the affected range. As a result, writeback
> of that folio may observe a hole. At writeback submit time,
> ext4_map_blocks() already handles this case and will not allocate
> blocks. However, when punch hole races with writeback, the
> following scenario can cause I/O completion to encounter a hole.
> 
>   punch hole                        writeback
>   ----------                        ---------
>   ext4_punch_hole()
>     ext4_truncate_page_cache_block_range()
>       iomap_invalidate_folio() [partial folio]
>         iomap_clear_range_dirty()
>         -- no ifs, sub-block dirty bits NOT cleared
>                                     ext4_iomap_writepages()
>                                       iomap_writepages()
>                                         ext4_iomap_writeback_submit()
>                                           ext4_iomap_map_writeback_range()
>                                             ext4_map_blocks(IO_SUBMIT)
>                                               -> extent exists, not a hole
>                                           submit_io()  -> bio in flight
>     down_write(&i_data_sem)
>     ext4_es_remove_extent()
>     ext4_ext_remove_space()
>       -> extent removed, hole inserted
>     up_write(&i_data_sem)
>     [ transaction commits, freed blocks released to buddy ]
>     [ allocator reuses freed blocks for another inode ]
>                                     [bio completes]
>                                     ext4_iomap_finish_ioend()
>                                       ext4_convert_unwritten_extents()
>                                         ext4_map_blocks(IO_CONVERT_EXT)
>                                           -> returns 0 (hole found)
>                                     [bio writes to reallocated blocks ]
>                                     -> silent data corruption

Hey Zhang,

Yes makes sense and I believe in case of bs < ps since we have already 
mkclean the to-be-partially-truncted folios, we won't have any user
process dirtying them again between writeback and freeing the blocks, so
the fix should be enough.

Feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>

Regards,
ojaswin

> 
> If the bio completes after the freed blocks have been reallocated to
> another inode, it silently overwrites them and causes data corruption.
> Therefore, on the iomap path, drain writeback in ext4_punch_hole() after
> partial zeroing and before starting the freeing transaction, so any bio
> in flight on a partial folio completes against blocks still owned by
> this inode before extent removal.  Once extent removal begins, the ES
> hole insert and the existing IO_SUBMIT short-circuit prevent further bio
> submission.
> 
> This is a workaround. The proper fix is for the iomap infrastructure to
> always maintain an ifs when folio size is larger than blocksize, so that
> partial invalidate reliably clears sub-block dirty bits and no draining
> is needed.
> 
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> ---
>  fs/ext4/inode.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index b876a8127d09..e91b4efce098 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5028,7 +5028,14 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>  	ret = ext4_zero_partial_blocks(inode, offset, length, &partial_zeroed);
>  	if (ret)
>  		return ret;
> -	if (((file->f_flags & O_SYNC) || IS_SYNC(inode)) && partial_zeroed) {
> +	/*
> +	 * On the iomap path, partial invalidate of a folio without an ifs
> +	 * leaves sub-block dirty bits uncleared.  Drain writeback before
> +	 * removing extents so that any bio in flight on a partial folio
> +	 * completes against blocks still owned by this inode.
> +	 */
> +	if (ext4_inode_buffered_iomap(inode) ||
> +	    (((file->f_flags & O_SYNC) || IS_SYNC(inode)) && partial_zeroed)) {
>  		ret = filemap_write_and_wait_range(inode->i_mapping, offset,
>  						   end - 1);
>  		if (ret)
> -- 
> 2.52.0
>