linux-kernel - Re: [PATCH v2 01/13] ext4: cleanup zeroout in ext4_split_extent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <06ab1faa-ea54-400e-9f21-46a20ac04403@huaweicloud.com>
Date: Fri, 28 Nov 2025 10:22:40 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
 yi.zhang@...wei.com, yizhang089@...il.com, libaokun1@...wei.com,
 yangerkun@...wei.com
Subject: Re: [PATCH v2 01/13] ext4: cleanup zeroout in ext4_split_extent_at()

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@...wei.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@...wei.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@...e.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
>>