Re: [PATCH -next v5 16/32] ext4: implement writeback path using iomap

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The iomap writeback path queues `iomap_ioend` structures to the `i_rsv_conversion_list`, but the worker function is uninitialized and defaults to `ext4_end_io_rsv_work`, leading to type confusion and memory corruption.
- [Critical] Failure to update `ext4_page_mkwrite()` to use the iomap framework for iomap-enabled inodes causes AB-BA deadlocks and type confusion on `folio->private`.
- [High] The iomap writeback range mapping logic unconditionally expands the allocation length up to the end of the writeback range, incorrectly allocating up to 8MB of unwritten extents into sparse file holes.
- [Medium] Missing WRITE_ONCE() for 64-bit ei->i_disksize allows torn reads on 32-bit systems.
- [Medium] Improper use of min_t() truncates the 64-bit wbc->range_end before comparison, defeating the maximum block cap.
--

--- Patch [16]: [PATCH -next v5 16/32] ext4: implement writeback path using iomap ---
commit 5c1a016c9a25acdff3a66fb714afe96821e567b6
Author: Zhang Yi <[email protected]>

ext4: implement writeback path using iomap

This commit adds the iomap writeback path for ext4 buffered I/O, introducing 
ext4_iomap_writepages() and the corresponding iomap_writeback_ops. It also 
adds an end I/O work handler to convert unwritten extents and update file 
size upon I/O completion.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d831d1911a6ff..0b3e54e12b78f 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
[ ... ]
> @@ -4134,10 +4135,154 @@ static void ext4_iomap_readahead(struct readahead_control *rac)
> +static int ext4_iomap_map_writeback_range(struct iomap_writepage_ctx *wpc,
> +					  loff_t offset, unsigned int dirty_len)
> +{
[ ... ]
> +	blk_len = dirty_len >> blkbits;
> +	blk_end = min_t(unsigned int, (wpc->wbc->range_end >> blkbits),
> +				      (UINT_MAX - 1));

[Severity: Medium]
Does this code truncate the 64-bit range_end before the comparison?

If wpc->wbc->range_end >> blkbits is evaluated as a 64-bit value but then
cast to unsigned int by min_t(unsigned int, ...), could this truncate the
value to 0 when the shifted value is a multiple of 2^32?

This might disable the writeback batching optimization for specific offsets
in very large files (e.g., syncing around 16TB boundaries).

> +	if (blk_end > index + blk_len)
> +		blk_len = blk_end - index + 1;

[Severity: High]
Will this logic unconditionally expand the block allocation length to the
end of the writeback range?

It looks like blk_len is expanded up to blk_end (which typically represents
the end of the VFS writeback range) without querying the delalloc
reservation length.

If a dirty folio is followed by a hole, could this allocate up to 8MB of
unwritten extents into sparse file holes? This seems like it could
improperly consume disk space and quotas.

> +retry:
> +	map.m_lblk = index;
> +	map.m_len = min_t(unsigned int, MAX_WRITEPAGES_EXTENT_LEN, blk_len);
[ ... ]
> +static const struct iomap_writeback_ops ext4_writeback_ops = {
> +	.writeback_range = ext4_iomap_writeback_range,
> +	.writeback_submit = ext4_iomap_writeback_submit,
> +};
> +
>  static int ext4_iomap_writepages(struct address_space *mapping,
>  				 struct writeback_control *wbc)
>  {

[Severity: Critical]
Since this patch introduces the iomap writeback path, do we also need to
update the mmap fault handler?

The ext4_file_vm_ops.page_mkwrite handler remains hardcoded to
ext4_page_mkwrite(), which uses the legacy ext4_block_page_mkwrite()
logic.

That legacy path attaches a buffer_head to folio->private and starts a
transaction before taking the folio lock.

When ext4_iomap_writepages() later flushes the folio,
iomap_writeback_init() will cast folio->private to struct
iomap_folio_state.

Calling atomic_set() on this state could overwrite the b_this_page list
pointer in the buffer_head, potentially leading to memory corruption and
AB-BA deadlocks between page faulting and writeback.

> -	return 0;
> +	struct inode *inode = mapping->host;
[ ... ]
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 0236b6b9785a2..2888e00575615 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
[ ... ]
> @@ -547,3 +548,124 @@ void ext4_bio_write_folio(struct ext4_io_submit *io, struct folio *folio,
> +static int ext4_iomap_wb_update_disksize(handle_t *handle, struct inode *inode,
> +					 loff_t end)
> +{
[ ... ]
> +	down_write(&ei->i_data_sem);
> +	new_disksize = min(new_disksize, i_size_read(inode));
> +	if (new_disksize > ei->i_disksize)
> +		ei->i_disksize = new_disksize;

[Severity: Medium]
Does this 64-bit assignment need a WRITE_ONCE() wrapper?

On 32-bit platforms, a plain 64-bit store to ei->i_disksize can be split
into two 32-bit stores.

A concurrent lockless reader calling READ_ONCE(ei->i_disksize) might
observe a torn value, resulting in wildly incorrect disk size values
during concurrent writeback and truncation.

> +	up_write(&ei->i_data_sem);
> +	ret = ext4_mark_inode_dirty(handle, inode);
[ ... ]
> +void ext4_iomap_end_bio(struct bio *bio)
> +{
> +	struct iomap_ioend *ioend = iomap_ioend_from_bio(bio);
> +	struct ext4_inode_info *ei = EXT4_I(ioend->io_inode);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
> +	if (list_empty(&ei->i_rsv_conversion_list))
> +		queue_work(EXT4_SB(ioend->io_inode->i_sb)->rsv_conversion_wq,
> +			   &ei->i_rsv_conversion_work);

[Severity: Critical]
Is the worker function for ei->i_rsv_conversion_work properly initialized
for iomap inodes?

It looks like ei->i_rsv_conversion_work is statically initialized to the
legacy ext4_end_io_rsv_work() function in ext4_alloc_inode().

This new iomap writeback path queues iomap_ioend structs to the list, but
it doesn't appear to call INIT_WORK() to reassign the worker function to
the newly introduced ext4_iomap_end_io().

When the work queue executes, wouldn't the legacy ext4_end_io_rsv_work()
iterate over this list and blindly cast iomap_ioend structs to
incompatible ext4_io_end_t structs, resulting in type confusion?

> +	list_add_tail(&ioend->io_list, &ei->i_rsv_conversion_list);
> +	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16