Re: [PATCH v2] ext4: don't report delalloc data as a hole on indirect-mapped inodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Caching a hole in the extent status tree when ext4_get_branch() fails due to an I/O error.

Pre-existing issues:
- [Critical] Transient -ENOMEM or -EFSCORRUPTED errors in ext4_get_branch() cause existing indirect blocks to be orphaned and overwritten.
--

commit 4a371ba0722d0395bf8f94f94d508b0175eed067
Author: Daejun Park <daejun7.park@samsung.com>

ext4: don't report delalloc data as a hole on indirect-mapped inodes

When a plain lookup finds no block, ext4_ind_map_blocks() currently
sizes the hole without considering delayed allocation, which causes
delayed data to be reported as a plain hole. This commit reconciles
the hole with the extent status tree by factoring out
ext4_determine_insert_hole() and using it for indirect-mapped inodes.
Indirect-mapped inodes now cache the holes they report.

Closes: https://lore.kernel.org/linux-ext4/594c17d9-c00f-e485-96fb-cedf27ce3aa3@ispras.ru/

[ ... ]

> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index 5aec759eed705..9dc0759c88ef4 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -586,6 +586,17 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
>  		for (i = partial - chain + 1; i < depth; i++)
>  			count = count * epb + (epb - offsets[i] - 1);
>  		count++;
> +
> +		/*
> +		 * The count knows nothing about delayed allocation, so let
> +		 * the common helper reconcile it with the extent status
> +		 * tree.  Clamp it first: with a large block size a subtree
> +		 * can be bigger than the logical block space.
> +		 */
> +		count = umin(count, EXT_MAX_BLOCKS - map->m_lblk);
> +		count = ext4_determine_insert_hole(inode, map->m_lblk,
> +						   map->m_lblk, count);

[Severity: Critical]
If ext4_get_branch() failed with an error like -EIO, -ENOMEM, or
-EFSCORRUPTED, does calling ext4_determine_insert_hole() here incorrectly
cache this transient error as a permanent hole in the extent status tree?

If (flags & EXT4_GET_BLOCKS_CREATE) == 0, the code enters this branch before
checking err, so subsequent reads might find this cached hole and return
zeros instead of retrying the disk read.

> +
>  		/* Fill in size of a hole we found */
>  		map->m_pblk = 0;
>  		map->m_len = umin(map->m_len, count);

[Severity: Critical]
This is a pre-existing issue, but looking just below this hunk in
ext4_ind_map_blocks():

	/* Failed read of indirect block */
	if (err == -EIO)
		goto cleanup;

	/*
	 * Okay, we need to do block allocation.
	*/

Does this check fail to handle errors other than -EIO from
ext4_get_branch()?

If ext4_get_branch() fails with -ENOMEM or -EFSCORRUPTED when
EXT4_GET_BLOCKS_CREATE is set, the code falls through to block allocation.
Will this cause existing indirect blocks to be orphaned and overwritten,
leading to a disk space leak and permanent replacement of valid file data
with zeros?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827043928epcms2p54597035aadd1eca0556ae9b0585694f9@epcms2p5?part=1