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

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.

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

> +
> +	/* 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);

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