Re: [PATCH v7 08/31] ext4: skip block allocation for holes in the data submission path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: silent data loss in atomic extent conversion due to unhandled 0 return
--

commit d986a085e4ae32e7076239e912abb3b6e2e97038
Author: Zhang Yi <yi.zhang@huawei.com>

ext4: skip block allocation for holes in the data submission path

When ext4_map_blocks() is called from the data submission path and I/O
end extent conversion path, it should not allocate blocks if the lookup
returns a hole. This patch prepares for the buffered iomap writeback
conversion by filtering out holes and bailing out with an error.

>     In the ioend extent conversion path, holes are also not expected because
>     we should wait for folio writeback before punching hole. If one is
>     encountered, it likely indicates a failure in the concurrency
>     protection, so ext4_map_blocks() returns zero, and we keep warning and
>     bail out with -EINVAL to surface the failure rather than continuing
>     conversion on torn data. Atomic writes in
>     ext4_convert_unwritten_extents_atomic() already bail out similarly.

[Severity: High]
The commit message states that ext4_convert_unwritten_extents_atomic()
"already bails out similarly" (presumably with an error code like -EINVAL)
when ext4_map_blocks() returns 0 for a hole.

However, looking at ext4_convert_unwritten_extents_atomic() in
fs/ext4/extents.c, it breaks the loop when ret is 0, and returns 0 (success):

fs/ext4/extents.c:ext4_convert_unwritten_extents_atomic() {
    ...
	while (ret >= 0 && ret < max_blocks) {
		map.m_lblk += ret;
		map.m_len = (max_blocks -= ret);
		ret = ext4_map_blocks(handle, inode, &map, flags);
		...
		if (ret <= 0)
			break;
	}
    ...
	if (ret <= 0 || ret2)
		ext4_warning(inode->i_sb, ...);

	return ret > 0 ? ret2 : ret;
}

If ret is 0, the expression ret > 0 ? ret2 : ret evaluates to 0.

Could this cause a regression where the unwritten extent conversion fails
due to encountering a hole, but the direct I/O layer is told it succeeded,
leaving unwritten extents on disk that will return zeroes on subsequent
reads?

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6b0489cbb1319..778cf539f6df2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c

[ ... ]

> @@ -830,6 +830,13 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  				map->m_flags |= EXT4_MAP_MAPPED;
>  			goto out;
>  		}
> +	} else if (retval == 0) {
> +		/*
> +		 * Do not allocate blocks for holes in the context of
> +		 * data submission path.
> +		 */
> +		if (!map->m_flags && (flags & EXT4_GET_BLOCKS_IO_SUBMIT))
> +			goto out;
>  	}
>  
>  	if (!handle) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009103033.2920530-1-yi.zhang@huaweicloud.com?part=8