Re: [PATCH v6 17/31] ext4: implement partial block zero range path using iomap

[Ojaswin Mujoo]

On Thu, Sep 03, 2026 at 08:35:29PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> Introduce a new iomap_ops instance, ext4_iomap_zero_ops, along with
> ext4_iomap_block_zero_range() to implement block zeroing via the iomap
> infrastructure for ext4.
> 
> ext4_iomap_block_zero_range() calls iomap_zero_range() with
> ext4_iomap_zero_begin() as the callback. The callback locates the
> range and populates the iomap mapping. If the range is mapped,
> iomap_zero_iter() in the iomap core zeros the partial block
> directly. If the range is an unwritten extent within EOF, the
> callback collects a dirty folio batch via iomap_fill_dirty_folios()
> so that iomap_zero_iter() can zero those folios directly, bypassing
> a separate slow flush operation that would otherwise be needed to
> convert the unwritten extent.
> 
> Note that ext4_iomap_zero_begin() can race with concurrent writeback:
> after it queries an unwritten extent, writeback may convert it to
> written and complete on the folio before iomap_fill_dirty_folios() scans
> the range. The empty batch then makes iomap_zero_iter() skip zeroing,
> leaving stale on-disk data.
> 
>   zero_range                      writeback
>   ----------------------          ----------------------
>   ext4_block_zero_range()
>     iomap_zero_range()
>       iomap_iter()
>         ext4_iomap_zero_begin()
>           ext4_iomap_map_blocks()
>             -> extent is UNWRITTEN
>                                   ext4_convert_unwritten_extents_endio()
>                                     -> extent is converted to WRITTEN
>                                     -> folio is clean
>           iomap_fill_dirty_folios()
>             filemap_get_folios_dirty()
>             -> folio is clean, not added to batch
>           ext4_set_iomap() -> IOMAP_UNWRITTEN
>       iomap_zero_iter()
>         __iomap_get_folio() -> NULL (empty batch)
>         iomap_iter_advance_full()  <-- zeroing skipped
> 
>   [later read returns stale on-disk data]   <--   CORRUPTION

Hmm so I checked the xfs side of it and basically, in ext4, we check the
mapping under i_data_sem and then get the dirty/under-writeback folios
without the i_data_sem, that's what causes the race you pointed. In XFS,
we do the extent lookup and fill folios under the i_data_sem equivalent
lock. This ensures that the mapping is unwritten when iomap_fill_dirty_folio() 
is called, which means we will definitely find folios in it if they are
in dirty or locked or writeback state.

It'll be pretty intrusive to implement xfs's solution in ext4 so for now
this retry logic seems good. Further, i_es_seq is incremented before
writeback is cleared so if we find no dirty/writeback folio and the seq
count is same we can be sure that it will not have issues. So all in all
looks good. Feel free to all:

Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>

Regards,
ojaswin

> 
> Therefore, we retry the extent lookup when iomap_fill_dirty_folios()
> adds nothing and the i_es_seq cookie captured at the first lookup has
> advanced, indicating the race actually occurred.
> 
> Other important constraints:
> 
> Zeroing out under an active journal handle can cause deadlock, as the
> lock/handle ordering is inconsistent with the iomap writeback path.
> Therefore, ext4_iomap_block_zero_range() must not be called under an
> active handle. In addition, for post-EOF zeroing, the caller cannot
> rely on data=ordered mode to persist the zeroed data before
> i_disksize is updated.
> 
> Subsequent patches will address this by deferring i_disksize update
> to i_size until after the zeroed data has been written back.
> 
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> ---
>  fs/ext4/inode.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 88d9b48ee828..b876a8127d09 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4108,6 +4108,67 @@ static int ext4_iomap_buffered_da_write_end(struct inode *inode, loff_t offset,
>  	return 0;
>  }
>  
> +static int ext4_iomap_zero_begin(struct inode *inode,
> +		loff_t offset, loff_t length, unsigned int flags,
> +		struct iomap *iomap, struct iomap *srcmap)
> +{
> +	struct iomap_iter *iter = container_of(iomap, struct iomap_iter, iomap);
> +	struct ext4_map_blocks map;
> +	u8 blkbits = inode->i_blkbits;
> +	unsigned int iomap_flags;
> +	int ret;
> +
> +	ret = ext4_emergency_state(inode->i_sb);
> +	if (unlikely(ret))
> +		return ret;
> +
> +	if (WARN_ON_ONCE(!(flags & IOMAP_ZERO)))
> +		return -EINVAL;
> +
> +again:
> +	ret = ext4_iomap_map_blocks(inode, offset, length, &map, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Look up dirty folios for unwritten mappings within EOF. Providing
> +	 * this bypasses the flush iomap uses to trigger extent conversion
> +	 * when unwritten mappings have dirty pagecache in need of zeroing.
> +	 */
> +	iomap_flags = 0;
> +	if (map.m_flags & EXT4_MAP_UNWRITTEN) {
> +		loff_t start = ((loff_t)map.m_lblk) << blkbits;
> +		loff_t end = ((loff_t)map.m_lblk + map.m_len) << blkbits;
> +		unsigned int count;
> +
> +		count = iomap_fill_dirty_folios(iter, &start, end,
> +						&iomap_flags);
> +		if ((start >> blkbits) < map.m_lblk + map.m_len)
> +			map.m_len = (start >> blkbits) - map.m_lblk;
> +
> +		/*
> +		 * This can be raced by a concurrent writeback that cleans
> +		 * the folio and converts the unwritten extent to written.
> +		 * Recheck the mapping after a folio lock round in
> +		 * iomap_fill_dirty_folios().
> +		 */
> +		if (count == 0 &&
> +		    map.m_seq != READ_ONCE(EXT4_I(inode)->i_es_seq))
> +			goto again;
> +	}
> +
> +	ext4_set_iomap(inode, iomap, &map, offset, length, flags);
> +	iomap->flags |= iomap_flags;
> +
> +	return 0;
> +}
> +
> +static DEFINE_IOMAP_ITER_NEXT(ext4_iomap_zero_next, ext4_iomap_zero_begin);
> +
> +static const struct iomap_ops ext4_iomap_zero_ops = {
> +	.iomap_next = ext4_iomap_zero_next,
> +};
> +
>  /*
>   * Since we always allocate unwritten extents, there is no need for
>   * iomap_end to clean up allocated blocks on a short write.
> @@ -4569,6 +4630,48 @@ static int ext4_block_journalled_zero_range(struct inode *inode, loff_t from,
>  	return err;
>  }
>  
> +static int ext4_block_iomap_zero_range(struct inode *inode, loff_t from,
> +				       loff_t length, bool *did_zero,
> +				       bool *zero_written)
> +{
> +	int ret;
> +
> +	/*
> +	 * Zeroing out under an active handle can cause deadlock since
> +	 * the order of acquiring the folio lock and starting a handle is
> +	 * inconsistent with the iomap writeback procedure.
> +	 */
> +	if (WARN_ON_ONCE(ext4_handle_valid(journal_current_handle())))
> +		return -EINVAL;
> +
> +	/* The zeroing scope should not extend across a block. */
> +	if (WARN_ON_ONCE((from >> inode->i_blkbits) !=
> +			 ((from + length - 1) >> inode->i_blkbits)))
> +		return -EINVAL;
> +
> +	if (!(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS) &&
> +	    !(inode_state_read_once(inode) & (I_NEW | I_FREEING)))
> +		WARN_ON_ONCE(!inode_is_locked(inode) &&
> +			!rwsem_is_locked(&inode->i_mapping->invalidate_lock));
> +
> +	ret = iomap_zero_range(inode, from, length, did_zero,
> +			       &ext4_iomap_zero_ops, &ext4_iomap_write_ops,
> +			       NULL);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * TODO: The iomap does not distinguish between different types
> +	 * of zeroing operations. So we always set zero_written whenever
> +	 * zeroing is performed, which may cause unnecessary folio
> +	 * flushing when zeroing occurs on delayed-allocated blocks.
> +	 */
> +	if (did_zero && zero_written)
> +		*zero_written = *did_zero;
> +
> +	return 0;
> +}
> +
>  /*
>   * Zeros out a mapping of length 'length' starting from file offset
>   * 'from'.  The range to be zero'd must be contained with in one block.
> @@ -4595,6 +4698,9 @@ static int ext4_block_zero_range(struct inode *inode,
>  	} else if (ext4_should_journal_data(inode)) {
>  		return ext4_block_journalled_zero_range(inode, from, length,
>  							did_zero);
> +	} else if (ext4_inode_buffered_iomap(inode)) {
> +		return ext4_block_iomap_zero_range(inode, from, length,
> +						   did_zero, zero_written);
>  	}
>  	return ext4_block_do_zero_range(inode, from, length, did_zero,
>  					zero_written);
> @@ -4655,6 +4761,10 @@ int ext4_block_zero_eof(struct inode *inode, loff_t from, loff_t end)
>  	 * concurrent writeback that updates i_disksize. And if such a race
>  	 * occurs, it means the previous unaligned EOF block has already been
>  	 * zeroed (if needed) and persisted to disk.
> +	 *
> +	 * TODO: In the iomap path, handle this by tracking the ordered range
> +	 * and updating i_disksize to i_size after the zeroed data has been
> +	 * written back.
>  	 */
>  	if (ext4_should_order_data(inode) &&
>  	    did_zero && zero_written && !IS_DAX(inode) &&
> -- 
> 2.52.0
>