Re: [PATCH v6 15/31] ext4: implement writeback path using iomap

[Zhang Yi <yizhang089@gmail.com>]

On 9/30/2026 4:51 PM, Ojaswin Mujoo wrote:
> On Sat, Sep 12, 2026 at 04:28:52PM +0800, Zhang Yi wrote:
>> On 9/3/2026 9:36 PM, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>>> - [Critical] Journal Credits Exhaustion / Panic in `ext4_iomap_finish_ioend()` due to arbitrarily large merged ioends.
>>> - [High] Infinite loop in `ext4_iomap_map_writeback_range` due to open-coded ENOSPC retry logic without limits.
>>> --
>>>
>>> commit e7cc5164579a940550c93afb2dfec1f186e079ed
>>> 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
>>> new writeback operations and end I/O handlers. It maps and allocates blocks
>>> as unwritten extents during writeback to prevent stale data exposure.
>>>
>>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>>> index 1f18854b9cdd3..f7d7610197a1c 100644
>>>> --- a/fs/ext4/inode.c
>>>> +++ b/fs/ext4/inode.c
>>>
>>> [ ... ]
>>>
>>>> +static int ext4_iomap_map_writeback_range(struct iomap_writepage_ctx *wpc,
>>>> +					  loff_t offset, unsigned int dirty_len)
>>>> +{
>>>
>>> [ ... ]
>>>
>>>> +retry:
>>>> +	map.m_lblk = index;
>>>> +	map.m_len = min_t(unsigned int, MAX_WRITEPAGES_EXTENT_LEN, blk_len);
>>>> +	ret = ext4_map_blocks(NULL, inode, &map,
>>>> +			      EXT4_GET_BLOCKS_CREATE_UNWRIT_EXT |
>>>> +			      EXT4_GET_BLOCKS_METADATA_NOFAIL |
>>>> +			      EXT4_GET_BLOCKS_IO_SUBMIT |
>>>> +			      EXT4_EX_NOCACHE);
>>>> +	if (ret < 0) {
>>>> +		if (ext4_emergency_state(sb))
>>>> +			return ret;
>>>> +
>>>> +		/*
>>>> +		 * Retry transient ENOSPC errors, if
>>>> +		 * ext4_count_free_blocks() is non-zero, a commit
>>>> +		 * should free up blocks.
>>>> +		 */
>>>> +		if (ret == -ENOSPC && journal && ext4_count_free_clusters(sb)) {
>>>> +			jbd2_journal_force_commit_nested(journal);
>>>> +			goto retry;
>>>> +		}
>>>
>>> [Severity: High]
>>> Could this open-coded retry loop result in an infinite hang under certain
>>> conditions?
>>>
>>> If a non-root user fills the disk up to the threshold of root-reserved
>>> blocks, ext4_map_blocks() will return -ENOSPC because the block allocator
>>> denies access to reserved blocks for non-root users.
>>>
>>> However, ext4_count_free_clusters() returns the total free space including
>>> those root-reserved blocks. Because the count evaluates as non-zero, the code
>>> will force a journal commit and goto retry indefinitely, without any loop
>>> limits.
>>>
>>> Should this retry logic use a limit like ext4_should_retry_alloc(), or
>>> specifically account for root-reserved blocks to prevent a soft lockup in
>>> the writeback worker thread?
>>>
>>
>> This logic is inherited from the existing buffer_head writeback path
>> (the -ENOSPC handling in ext4_do_writepages()), which also retries after
>> a forced commit without any limit, so it does not introduce a new class
>> of problem.  That said, I agree that bounding the number of retries is
>> better, and I will add a limit in the next version.
>>
>>
>>>> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
>>>> index 0236b6b9785a2..9b0e12b5463cf 100644
>>>> --- a/fs/ext4/page-io.c
>>>> +++ b/fs/ext4/page-io.c
>>>
>>> [ ... ]
>>>
>>>> +static void ext4_iomap_finish_ioend(struct iomap_ioend *ioend)
>>>> +{
>>>
>>> [ ... ]
>>>
>>>> +	/*
>>>> +	 * We may need to convert one extent, update the i_disksize and
>>>> +	 * dirty the inode.
>>>> +	 */
>>>> +	credits = ext4_chunk_trans_blocks(inode,
>>>> +			EXT4_MAX_BLOCKS(size, pos, inode->i_blkbits));
>>>> +	handle = ext4_journal_start(inode, EXT4_HT_EXT_CONVERT, credits);
>>>> +	if (IS_ERR(handle)) {
>>>> +		ret = PTR_ERR(handle);
>>>> +		goto out_err;
>>>> +	}
>>>
>>> [ ... ]
>>>
>>>> +void ext4_iomap_end_io(struct work_struct *work)
>>>> +{
>>>> +	struct ext4_inode_info *ei = container_of(work, struct ext4_inode_info,
>>>> +						  i_rsv_conversion_work);
>>>> +	struct iomap_ioend *ioend;
>>>> +	struct list_head ioend_list;
>>>> +	unsigned long flags;
>>>> +
>>>> +	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
>>>> +	list_replace_init(&ei->i_rsv_conversion_list, &ioend_list);
>>>> +	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
>>>> +
>>>> +	iomap_sort_ioends(&ioend_list);
>>>> +	while (!list_empty(&ioend_list)) {
>>>> +		ioend = list_entry(ioend_list.next, struct iomap_ioend, io_list);
>>>> +		list_del_init(&ioend->io_list);
>>>> +		iomap_ioend_try_merge(ioend, &ioend_list);
>>>> +		ext4_iomap_finish_ioend(ioend);
>>>> +	}
>>>> +}
>>>
>>> [Severity: Critical]
>>> Is there a risk of exhausting journal credits and panicking JBD2 when ioends
>>> are merged?
>>>
>>> In ext4_iomap_end_io(), iomap_ioend_try_merge() merges contiguous completions
>>> into an arbitrarily large ioend. When this merged ioend is later passed to
>>> ext4_iomap_finish_ioend(), it requests credits using ext4_chunk_trans_blocks().
>>>
>>> However, ext4_chunk_trans_blocks() internally hardcodes the extent count to 1,
>>> allocating credits sufficient for modifying only a single extent on disk.
>>>
>>> If a user writes a very large contiguous file (e.g., larger than 128MB), the
>>> merged ioend will exceed the maximum size of a single unwritten extent in ext4
>>> (which is 32,768 blocks). This will cause ext4_convert_unwritten_extents() to
>>> loop over multiple extents using the same transaction handle.
>>>
>>> Could this exhaust the single-extent credits provided by the transaction
>>> handle, resulting in a JBD2 panic during writeback?
>>>
>>
>> This is right.  ext4_convert_unwritten_extents() converts every extent
>> covered by the ioend using a single transaction handle, but that handle
>> was sized by ext4_chunk_trans_blocks() for one extent only, so a large
>> enough merged ioend can run out of credits.  This is hard to hit in
>> practice, since the end I/O worker normally runs promptly and not many
>> completed ioends accumulate before the merge, but to be safe I will
>> still fix it: call ext4_journal_ensure_credits() before each extent in
>> the conversion loop, so that the transaction is extended, or restarted
>> once it is full, as needed, and correct the estimation to
>> ext4_meta_trans_blocks(inode, max_blocks, 1, 0) at the same time, as no
>> data blocks are allocated there.
> 
> Yes I agree that it seems like a valid issue. I'm a bit unfamiliar
> with the journal ensure credits logic but seems like it would be
> stopping the running txn and starting a new transaction right?

Yes, the change is also very simple, please see:

https://github.com/zhangyi089/linux/commit/d8078ca0dd93bd92c0fa0904e1c76b112339f48d

> 
> If so, maybe lets move the idisksize update after the the conversion so
> that its updated in the final transaction, once the conversion is
> complete, which is a bit cleaner.
> 

That won't work. Since the conversion may be split across different
transactions, a single transaction can no longer provide the atomicity
guarantee for both the conversion and the i_disksize update. If we do
the conversion first and then lose power after some of the conversion
transactions have committed, we would end up with written extents beyond
EOF, which would make fsck complain.

Thanks,
Yi.