Re: [PATCH v7 14/31] ext4: rework handle credit accounting for unwritten extent conversion

[Zhang Yi <yizhang089@gmail.com>]

On 10/9/2026 7:20 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ext4: atomicity violation in fallocate extent conversion
> --
> 
> commit 352cf8eb3125c707efc06205d4a61cc670cd67bf
> Author: Zhang Yi <yi.zhang@huawei.com>
> 
> ext4: rework handle credit accounting for unwritten extent conversion
> 
> This commit updates the unwritten extent conversion process to handle long
> merged ioends. It introduces a check using ext4_journal_ensure_credits to
> extend or restart the transaction if credits run low during the loop.
> 
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 76038b6c36552..83d040e36b682 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
> [ ... ]
>> @@ -5137,27 +5137,36 @@ int ext4_convert_unwritten_extents(handle_t *handle, struct inode *inode,
> [ ... ]
>>   	while (max_blocks) {
>> -		if (credits) {
>> -			handle = ext4_journal_start(inode, EXT4_HT_MAP_BLOCKS,
>> -						    credits);
>> -			if (IS_ERR(handle)) {
>> -				ret = PTR_ERR(handle);
>> -				break;
>> -			}
>> -		}
>> +		/*
>> +		 * The caller cannot know how many extents the range covers,
>> +		 * so make sure the transaction can take one more extent,
>> +		 * extending or restarting it once it is full.  The reserved
>> +		 * handle of the buffer_head writeback path is safe as it has
>> +		 * enough credits for the conversion, so it is not restarted
>> +		 * here.
>> +		 */
>> +		ret = ext4_journal_ensure_credits(handle, credits, 0);
>> +		if (ret < 0)
>> +			break;
> 
> [Severity: High]
> Does restarting the caller's transaction here break atomicity requirements
> for fallocate?
> 
> ext4_alloc_file_blocks relies on the atomicity of converting unwritten
> extents to written and adding the inode to the orphan list. This prevents
> e2fsck from flagging initialized extents beyond i_disksize as corruption on
> crash.
> 
> If the transaction restarts during this conversion loop, the first batch of
> converted extents is committed to the journal before the loop finishes and
> ext4_orphan_add is called in the parent function:
> 
> fs/ext4/extents.c:ext4_alloc_file_blocks() {
>      ...
>      ret = ext4_convert_unwritten_extents(handle, inode, ...);
>      ...
>      if (new_size && converted) {
>          ret2 = ext4_orphan_add(handle, inode);
>      ...
> }
> 
> If a crash occurs after the transaction restarts but before ext4_orphan_add
> completes, will it leave the disk with initialized extents beyond i_disksize
> and no orphan list entry, triggering filesystem corruption warnings and
> silent truncations in e2fsck?
> 

This is a pre-existing issue in ext4_alloc_file_blocks(), which is
extremely hard to trigger in practice, and the failure mode after the
patch will differ somewhat from before.

This can only be triggered by a size-extending FALLOC_FL_WRITE_ZEROES
that runs concurrently with another I/O completion. Since
ext4_alloc_file_blocks() holds i_rwsem, what it requires is that the
extent that the ext4_map_blocks() call at the top of the loop returned
as a single extent gets split before ext4_convert_unwritten_extents()
is called, so that the conversion is spread over more extents than the
extent tree had at map time. That means another unwritten extent
conversion has to land in that window, and the io_end conversion runs
from a workqueue, so it routinely completes only after the data
writeback and after the page cache has been dropped. A direct I/O
completion (ext4_dio_write_end_io()) can do the same. Buffered writes,
truncate and other fallocate calls are excluded by i_rwsem.

Before the patch, in the worst case, this would exhaust the handle
credits and cause a filesystem error. After the patch, since
ext4_journal_ensure_credits() will first try to extend the credits, the
credit exhaustion itself becomes much less likely, and only when the
extension fails, because the transaction is already at
j_max_transaction_buffers, will the transaction be restarted, which then
can trigger the issue Sashiko described.

This issue needs to be fixed separately, perhaps by moving
ext4_orphan_add() in ext4_alloc_file_blocks() before
ext4_convert_unwritten_extents().

Thanks,
Yi.