Re: [PATCH 3/4] ext4: add inode to orphan list before extent conversion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: silent false success on ENOSPC retry in ext4_alloc_file_blocks()
--

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

ext4: add inode to orphan list before extent conversion

This commit moves the ext4_orphan_add() call in ext4_alloc_file_blocks()
before ext4_convert_unwritten_extents() so that transaction restarts during
extent conversion do not leave written extents beyond i_disksize uncovered by
the orphan list. It also adjusts the post-conversion progress and cleanup
handling when conversion fails before converting any blocks.

> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 7193429a05e1f..677749f0c9c68 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4701,6 +4701,23 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>  				break;
>  			}
>  
> +			/*
> +			 * Add the inode to the orphan list before the
> +			 * conversion, as it may restart the transaction and
> +			 * commit the extents converted so far.  Written
> +			 * extents must stay covered by the orphan list until
> +			 * i_disksize catches up.
> +			 */
> +			if (new_size) {
> +				ret2 = ext4_orphan_add(handle, inode);
> +				orphan = true;
> +				if (unlikely(ret2)) {
> +					ret = ret2;
> +					ext4_journal_stop(handle);
> +					break;
> +				}
> +			}
> +
>  			ret = ext4_convert_unwritten_extents(handle,
>  					inode, (loff_t)map.m_lblk << blkbits,
>  					(loff_t)map.m_len << blkbits,
> @@ -4708,23 +4725,19 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>  			if (ret)
>  				map.m_len = converted;
>  
> -			/*
> -			 * If blocks beyond i_disksize are converted, add
> -			 * the inode to the orphan list and advance the epos.
> -			 */
> -			if (new_size && converted) {
> -				ret2 = ext4_orphan_add(handle, inode);
> -				ret = ret ? ret : ret2;
> -				orphan = true;
> -			}
> -
>  			ret3 = ext4_journal_stop(handle);
>  			ret = ret ? ret : ret3;
>  		}
>  
> -		map.m_lblk += map.m_len;
> -		map.m_len = len_lblk = len_lblk - map.m_len;
> -		epos = EXT4_LBLK_TO_B(inode, map.m_lblk);
> +		/*
> +		 * The conversion may have failed without converting
> +		 * anything.
> +		 */
> +		if (map.m_len) {
> +			map.m_lblk += map.m_len;
> +			map.m_len = len_lblk = len_lblk - map.m_len;
> +			epos = EXT4_LBLK_TO_B(inode, map.m_lblk);
> +		}
>  		if (ret)
>  			break;
>  	}
> @@ -4732,12 +4745,13 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>  	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>  		goto retry;

[Severity: High]
What happens on the goto retry path if ext4_convert_unwritten_extents() fails
with -ENOSPC and converted == 0?

When ret is non-zero and converted is 0, map.m_len is set to 0:

	if (ret)
		map.m_len = converted;

Previously, map.m_len = len_lblk = len_lblk - map.m_len ran unconditionally,
which restored map.m_len to len_lblk when map.m_len was 0. With that update
now guarded by if (map.m_len), map.m_len remains 0 while len_lblk > 0.

If ret == -ENOSPC and ext4_should_retry_alloc() returns true, jumping to
retry calls ext4_map_blocks() with map.m_len == 0:

fs/ext4/extents.c:ext4_alloc_file_blocks() {
	...
	while (len_lblk) {
		...
		ret = ext4_map_blocks(handle, inode, &map, flags);
		if (ret <= 0) {
			...
			ext4_mark_inode_dirty(handle, inode);
			ext4_journal_stop(handle);
			break;
		}
}

Because map.m_lblk already maps to the unwritten extent allocated before the
retry, both the extent status tree lookup in ext4_map_blocks() and
ext4_ext_handle_unwritten_extents() clamp the returned block count to
map.m_len (0). That causes ext4_map_blocks() to return 0, breaking out of the
loop with ret == 0 and silently returning success to the caller without
converting the unwritten extent or finishing the remaining allocation.

Should map.m_len be reset to len_lblk when map.m_len is 0 (or before jumping
to retry)?

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