Re: [PATCH] ext4: do not advance i_size on a buffered write that copied nothing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: delayed block and quota leak on failed writes in delalloc path
--

commit acb80c0263f3e15c2b0d945f36271eb41aefe50e
Author: Hengyu Liang <hengyul@cs.unc.edu>

ext4: do not advance i_size on a buffered write that copied nothing

This commit changes the ext4 buffered write path to avoid updating i_size
when a write past EOF copies zero bytes. It also attempts to clean up the
page cache and delayed extents beyond i_size in the delalloc path.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 9a9ec02dce9fd..339b20a238053 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c

[ ... ]

> @@ -3210,6 +3213,24 @@ static int ext4_da_should_update_i_disksize(struct folio *folio,
>  	return 1;
>  }
>  
> +/*
> + * Delalloc counterpart of ext4_truncate_failed_write(): ->write_begin() only
> + * reserved delayed blocks, so drop the page cache and the delayed extents
> + * beyond i_size and leave the blocks that are allocated there alone.
> + */
> +static void ext4_da_truncate_failed_write(struct inode *inode)
> +{
> +	struct address_space *mapping = inode->i_mapping;
> +	ext4_lblk_t lblk = EXT4_B_TO_LBLK(inode, inode->i_size);

[Severity: High]
Does this EXT4_B_TO_LBLK() conversion round up the block number?

If i_size is not block-aligned, won't this skip the delayed extent reserved
for the block overlapping EOF, leaking its reservation since the folio is
never dirtied?

> +
> +	filemap_invalidate_lock(mapping);
> +	truncate_inode_pages(mapping, inode->i_size);
> +	down_write(&EXT4_I(inode)->i_data_sem);
> +	ext4_es_remove_extent(inode, lblk, EXT_MAX_BLOCKS - lblk);
> +	up_write(&EXT4_I(inode)->i_data_sem);
> +	filemap_invalidate_unlock(mapping);
> +}

[ ... ]

> @@ -3261,9 +3282,16 @@ static int ext4_da_do_write_end(struct address_space *mapping,
>  	folio_unlock(folio);
>  	folio_put(folio);
>  
> -	if (pos > old_size)
> +	if (copied && pos > old_size)
>  		pagecache_isize_extended(inode, old_size, pos);
>  
> +	/*
> +	 * A write past i_size that copied nothing leaves i_size alone, so
> +	 * trim what ->write_begin() set up beyond it.
> +	 */
> +	if (!copied && pos > old_size)
> +		ext4_da_truncate_failed_write(inode);

[Severity: High]
Does this condition miss cases where the write starts before or exactly at EOF
but extends past it?

If a failed write (copied == 0) starts exactly at EOF (pos == old_size) or
overlaps EOF (pos < old_size and pos + len > old_size), pos > old_size is
false. 

Since the write_begin path already reserved delayed blocks past i_size,
will skipping this cleanup permanently leak those reserved blocks and quota?

> +
>  	if (!disksize_changed)
>  		return copied;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009044656.1993196-1-hengyul@cs.unc.edu?part=1