Re: [PATCH v2 01/13] ext4: cleanup zeroout in ext4_split_extent_at()

[Zhang Yi <yi.zhang@huaweicloud.com>]

On 11/27/2025 8:02 PM, Jan Kara wrote:
> On Fri 21-11-25 14:07:59, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> zero_ex is a temporary variable used only for writing zeros and
>> inserting extent status entry, it will not be directly inserted into the
>> tree. Therefore, it can be assigned values from the target extent in
>> various scenarios, eliminating the need to explicitly assign values to
>> each variable individually.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> 
> Nice simplification. I'd just note that the new method copies also the
> unwritten state of the original extent to zero_ex (the old method didn't do
> this). It doesn't matter in this case but it might still be nice to add a
> comment about it before the code doing the copying. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza

Thank you a lot for reviewing this series! It seems that calling
ext4_ext_mark_initialized() after copying is also acceptable.

Cheers,
Yi.

> 
>> ---
>>  fs/ext4/extents.c | 63 ++++++++++++++++++-----------------------------
>>  1 file changed, 24 insertions(+), 39 deletions(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index c7d219e6c6d8..91682966597d 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -3278,46 +3278,31 @@ static struct ext4_ext_path *ext4_split_extent_at(handle_t *handle,
>>  	ex = path[depth].p_ext;
>>  
>>  	if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
>> -		if (split_flag & (EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
>> -			if (split_flag & EXT4_EXT_DATA_VALID1) {
>> -				err = ext4_ext_zeroout(inode, ex2);
>> -				zero_ex.ee_block = ex2->ee_block;
>> -				zero_ex.ee_len = cpu_to_le16(
>> -						ext4_ext_get_actual_len(ex2));
>> -				ext4_ext_store_pblock(&zero_ex,
>> -						      ext4_ext_pblock(ex2));
>> -			} else {
>> -				err = ext4_ext_zeroout(inode, ex);
>> -				zero_ex.ee_block = ex->ee_block;
>> -				zero_ex.ee_len = cpu_to_le16(
>> -						ext4_ext_get_actual_len(ex));
>> -				ext4_ext_store_pblock(&zero_ex,
>> -						      ext4_ext_pblock(ex));
>> -			}
>> -		} else {
>> -			err = ext4_ext_zeroout(inode, &orig_ex);
>> -			zero_ex.ee_block = orig_ex.ee_block;
>> -			zero_ex.ee_len = cpu_to_le16(
>> -						ext4_ext_get_actual_len(&orig_ex));
>> -			ext4_ext_store_pblock(&zero_ex,
>> -					      ext4_ext_pblock(&orig_ex));
>> -		}
>> +		if (split_flag & EXT4_EXT_DATA_VALID1)
>> +			memcpy(&zero_ex, ex2, sizeof(zero_ex));
>> +		else if (split_flag & EXT4_EXT_DATA_VALID2)
>> +			memcpy(&zero_ex, ex, sizeof(zero_ex));
>> +		else
>> +			memcpy(&zero_ex, &orig_ex, sizeof(zero_ex));
>>  
>> -		if (!err) {
>> -			/* update the extent length and mark as initialized */
>> -			ex->ee_len = cpu_to_le16(ee_len);
>> -			ext4_ext_try_to_merge(handle, inode, path, ex);
>> -			err = ext4_ext_dirty(handle, inode, path + path->p_depth);
>> -			if (!err)
>> -				/* update extent status tree */
>> -				ext4_zeroout_es(inode, &zero_ex);
>> -			/* If we failed at this point, we don't know in which
>> -			 * state the extent tree exactly is so don't try to fix
>> -			 * length of the original extent as it may do even more
>> -			 * damage.
>> -			 */
>> -			goto out;
>> -		}
>> +		err = ext4_ext_zeroout(inode, &zero_ex);
>> +		if (err)
>> +			goto fix_extent_len;
>> +
>> +		/* update the extent length and mark as initialized */
>> +		ex->ee_len = cpu_to_le16(ee_len);
>> +		ext4_ext_try_to_merge(handle, inode, path, ex);
>> +		err = ext4_ext_dirty(handle, inode, path + path->p_depth);
>> +		if (!err)
>> +			/* update extent status tree */
>> +			ext4_zeroout_es(inode, &zero_ex);
>> +		/*
>> +		 * If we failed at this point, we don't know in which
>> +		 * state the extent tree exactly is so don't try to fix
>> +		 * length of the original extent as it may do even more
>> +		 * damage.
>> +		 */
>> +		goto out;
>>  	}
>>  
>>  fix_extent_len:
>> -- 
>> 2.46.1
>>