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

[Zhang Yi <yizhang089@gmail.com>]

On 8/14/2026 6:19 PM, sashiko-bot@kernel.org wrote:
> 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 <yi.zhang@huawei.com>
> 
> 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;

It's true that this can indeed happen when wbc->end is manually
specified. We use umin() instead.

> 
> [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.
> 

This is a false positive. blk_len will be trimmed in
ext4_map_blocks().

>> +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.
> 

This is a false positive. The mmap fault handler changes are in
the next patch (patch 17).

>> -	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?
> 

This is a false positive. ei->i_rsv_conversion_work is initialized
in patch 31, where the iomap path is formally enabled.

Thanks,
Yi.

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