Re: [PATCH -next v5 15/32] ext4: implement buffered write path using iomap

On 8/14/2026 6:40 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Extending a file within an already-allocated block does not update the on-disk `i_disksize` or mark the inode dirty, causing appended data to be lost on a crash or unmount.
> - [High] The iomap non-delalloc buffered write path permanently leaks unwritten extents past `i_size` when a short write or failure occurs.
> --
> 
> --- Patch [15]: [PATCH -next v5 15/32] ext4: implement buffered write path using iomap ---
> commit 7a553e37ec457f0a485aa7c6fc378e68d6d66bfb
> Author: Zhang Yi <[email protected]>
> 
> ext4: implement buffered write path using iomap
> 
> This commit introduces two new iomap_ops instances for ext4 buffered writes:
> one for delayed allocation mode and another for non-delayed allocation mode.
> It also updates the folio lock and transaction start ordering for the new
> iomap buffered write path.
> 
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 9098d9a5fc05b..d831d1911a6ff 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
> 
> [ ... ]
> 
>> @@ -3994,6 +4009,113 @@ static int ext4_iomap_buffered_read_begin(struct inode *inode, loff_t offset,
>>   	return 0;
>>   }
>>   
>> +static int ext4_iomap_buffered_do_write_begin(struct inode *inode,
>> +		loff_t offset, loff_t length, unsigned int flags,
>> +		struct iomap *iomap, struct iomap *srcmap, bool delalloc)
>> +{
>> +	int ret, retries = 0;
>> +	struct ext4_map_blocks map;
>> +	int map_flags;
>> +
>> +	ret = ext4_emergency_state(inode->i_sb);
>> +	if (unlikely(ret))
>> +		return ret;
>> +
>> +	/* Inline data and non-extent are not supported. */
>> +	if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
>> +		return -ERANGE;
>> +	if (WARN_ON_ONCE(!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
>> +		return -EINVAL;
>> +	if (WARN_ON_ONCE(!(flags & IOMAP_WRITE)))
>> +		return -EINVAL;
>> +
>> +	map_flags = delalloc ? EXT4_GET_BLOCKS_DELALLOC_RESERVE :
>> +			       EXT4_GET_BLOCKS_CREATE_UNWRIT_EXT;
>> +retry:
>> +	ret = ext4_iomap_map_blocks(inode, offset, length, &map, map_flags);
> 
> [Severity: High]
> Does this code permanently leak unwritten extents past i_size if a short
> write or write failure occurs?
> 
> If the user memory copy subsequently fails (e.g. fault_in_iov_iter_readable()
> returns -EFAULT), it results in a short write.
> 
> In the non-delalloc path, ext4_iomap_buffered_write_ops lacks an iomap_end
> callback to clean up the allocated on-disk blocks, which could exhaust
> disk space or quota.

For short writes, leaving the preallocated blocks behind is not
really a problem. They are visible to users and can be reclaimed
manually, so it's not a permanent leak. I don't think we need to
fix this.

> 
>> +	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>> +		goto retry;
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ext4_set_iomap(inode, iomap, &map, offset, length, flags);
>> +	return 0;
>> +}
> 
> [ ... ]
> 
>> +static int ext4_iomap_buffered_da_write_end(struct inode *inode, loff_t offset,
>> +					    loff_t length, ssize_t written,
>> +					    unsigned int flags,
>> +					    struct iomap *iomap)
>> +{
>> +	loff_t start_byte, end_byte;
>> +
>> +	/* If we didn't reserve the blocks, we're not allowed to punch them. */
>> +	if (iomap->type != IOMAP_DELALLOC || !(iomap->flags & IOMAP_F_NEW))
>> +		return 0;
> 
> [Severity: Critical]
> Will this silently lose data for extending writes within already-allocated
> blocks?
> 
> If a buffered write appends data to a file within an extent that was already
> allocated, iomap_write_iter() will update the in-memory i_size and set
> IOMAP_F_SIZE_CHANGED.
> 
> However, this early return skips checking IOMAP_F_SIZE_CHANGED.
> 
> Without updating EXT4_I(inode)->i_disksize and calling ext4_mark_inode_dirty()
> to persist the size change, appended data can be lost on a crash.

This is a false concern. i_disksize is updated during writeback,
not in the iomep_end callback.

Thanks,
Yi.

> 
>> +
>> +	/* Nothing to do if we've written the entire delalloc extent */
>> +	start_byte = iomap_last_written_block(inode, offset, written);
>> +	end_byte = round_up(offset + length, i_blocksize(inode));
>> +	if (start_byte >= end_byte)
>> +		return 0;
>> +
>> +	filemap_invalidate_lock(inode->i_mapping);
>> +	iomap_write_delalloc_release(inode, start_byte, end_byte, flags,
>> +				     iomap, ext4_iomap_punch_delalloc);
>> +	filemap_invalidate_unlock(inode->i_mapping);
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Since we always allocate unwritten extents, there is no need for
>> + * iomap_end to clean up allocated blocks on a short write.
>> + */
>> +static DEFINE_IOMAP_ITER_NEXT(ext4_iomap_buffered_write_next,
>> +			      ext4_iomap_buffered_write_begin);
>> +
>> +const struct iomap_ops ext4_iomap_buffered_write_ops = {
>> +	.iomap_next = ext4_iomap_buffered_write_next,
>> +};
> 
> [Severity: High]
> Is it safe to omit an iomap_end callback here?
> 
> Without an iomap_end callback for the non-delalloc path, two issues appear
> to occur:
> 
> 1) Allocated unwritten extents past i_size are leaked if a write fails,
> exhausting disk space or quota.
> 
> 2) Extending writes into an already-allocated extent set IOMAP_F_SIZE_CHANGED
> in the core iomap code, but without an iomap_end callback to update i_disksize
> and mark the inode dirty, this size update is never persisted to disk.
> 
>> +
>> +static DEFINE_IOMAP_ITER_NEXT_END(ext4_iomap_buffered_da_write_next,
>> +				  ext4_iomap_buffered_da_write_begin,
>> +				  ext4_iomap_buffered_da_write_end);
>