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

[Zhang Yi <yizhang089@gmail.com>]

On 10/24/2026 1:25 AM, Hengyu Liang wrote:
> Commit 665575cff098 ("filemap: move prefaulting out of hot write path")
> made generic_perform_write() call ->write_begin() and ->write_end()
> before it finds out that the source buffer cannot be read.  In that
> case ->write_end() is called with copied == 0.
> 
> However, ext4_write_end(), ext4_journalled_write_end() and
> ext4_da_do_write_end() set i_size to pos + copied without checking
> copied.  As of now, a buffered write() past EOF that fails with EFAULT
> grows the file to the write offset, and the new size can reach the
> disk.
> 
> The issue can be reproduced on an ext4 mount with:
> 
>      char *p = mmap(NULL, 4096, PROT_NONE,
>                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>      int fd = open("/mnt/ext4/file", O_CREAT | O_TRUNC | O_RDWR, 0600);
>      pwrite(fd, p, 4096, 1024 * 1024);   /* fails with EFAULT */
>      fstat(fd, &st);
> 
> Before that commit, the result is st_size = 0.
> After it, the result is st_size = 1048576.
> 
> This patch will leave i_size alone when nothing was copied, as
> ext4_write_inline_data_end() already does.  The delalloc path does not
> trim after a failed write, so when such a write reaches past i_size,
> it also drops the folio and the delayed blocks that ->write_begin()
> set up there.  Otherwise that folio stays dirty past EOF and writeback
> never cleans it.

I feel the last sentence isn't quite accurate. Dirty folio past
EOF will be cleaned up during writeback. In mpage_process_page_bufs(),
when lblk >= blocks, if 1) there are no blocks to map, it will
directly call mpage_submit_folio(), which clears the dirty flag on
the folio, and then mpage_submit_folio() will shorten the writeback
length, so that ext4_bio_write_folio() will also clear the dirty
flag on the corresponding bh; if 2) there are blocks to map, then
after the required block allocation is done, it will likewise call
mpage_submit_folio() and do similar cleanup. Am I missing something?

> 
> Fixes: 665575cff098 ("filemap: move prefaulting out of hot write path")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---
> Changes in v2:
> - Rebase onto the ext4 dev branch.
> - Also trim when a failed write starts before EOF and ends past it
>    (pos + len > old_size), as suggested by Zhang Yi.
> - Keep the Fixes tag and the new helper, as discussed on v1.
> - v1: https://lore.kernel.org/r/20261009044656.1993196-1-hengyul@cs.unc.edu
> 
> Tested in KVM with KASAN: the reproducer above and failed writes around
> EOF on delalloc, nodelalloc, bigalloc and 1k block filesystems.
> e2fsck -fn reports no errors.
> 
>   fs/ext4/inode.c | 42 +++++++++++++++++++++++++++++++++++-------
>   1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6ede3644a738..e96321ce17d2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1578,15 +1578,18 @@ static int ext4_write_end(const struct kiocb *iocb,
>   	 * it's important to update i_size while still holding folio lock:
>   	 * page writeout could otherwise come in and zero beyond i_size.
>   	 *
> +	 * If nothing was copied (e.g. the source buffer was unreadable), do
> +	 * not move i_size up to pos.
> +	 *
>   	 * If FS_IOC_ENABLE_VERITY is running on this inode, then Merkle tree
>   	 * blocks are being written past EOF, so skip the i_size update.
>   	 */
> -	if (!verity)
> +	if (copied && !verity)
>   		i_size_changed = ext4_update_inode_size(inode, pos + copied);
>   	folio_unlock(folio);
>   	folio_put(folio);
>   
> -	if (old_size < pos && !verity)
> +	if (copied && old_size < pos && !verity)
>   		pagecache_isize_extended(inode, old_size, pos);
>   
>   	/*
> @@ -1697,13 +1700,13 @@ static int ext4_journalled_write_end(const struct kiocb *iocb,
>   		if (!partial)
>   			folio_mark_uptodate(folio);
>   	}
> -	if (!verity)
> +	if (copied && !verity)
>   		size_changed = ext4_update_inode_size(inode, pos + copied);
>   	EXT4_I(inode)->i_datasync_tid = handle->h_transaction->t_tid;
>   	folio_unlock(folio);
>   	folio_put(folio);
>   
> -	if (old_size < pos && !verity)
> +	if (copied && old_size < pos && !verity)
>   		pagecache_isize_extended(inode, old_size, pos);
>   
>   	if (size_changed) {
> @@ -3349,6 +3352,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);
> +
> +	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);
> +}
> +
>   static int ext4_da_do_write_end(struct address_space *mapping,
>   			loff_t pos, unsigned len, unsigned copied,
>   			struct folio *folio)
> @@ -3386,12 +3407,12 @@ static int ext4_da_do_write_end(struct address_space *mapping,
>   	 * checked, we need to update i_disksize here as certain
>   	 * ext4_writepages() paths not allocating blocks and update i_disksize.
>   	 */
> -	if (new_i_size > inode->i_size) {
> +	if (copied && new_i_size > inode->i_size) {
>   		unsigned long end;
>   
>   		i_size_write(inode, new_i_size);
>   		end = offset_in_folio(folio, new_i_size - 1);
> -		if (copied && ext4_da_should_update_i_disksize(folio, end)) {
> +		if (ext4_da_should_update_i_disksize(folio, end)) {
>   			ext4_update_i_disksize(inode, new_i_size);
>   			disksize_changed = true;
>   		}
> @@ -3400,9 +3421,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 reaching past i_size that copied nothing leaves i_size
> +	 * alone, so trim what ->write_begin() set up beyond it.
> +	 */
> +	if (!copied && pos + len > old_size)

Why do you keep the '!copied' condition? AFAICT, if copied is non-zero
but less than len, the excess delalloc extent allocate by
ext4_da_write_begin() should still be cleaned up.

Thanks,
Yi.

> +		ext4_da_truncate_failed_write(inode);
> +
>   	if (!disksize_changed)
>   		return copied;
>   
> 
> base-commit: 8c4f6e3376348fc3162eed86a05b933921c8f9ed