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>
---
 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

On 2025-11-21 14:07, 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 cleanup! Feel free to add:

Reviewed-by: Baokun Li <libaokun1@huawei.com>

> ---
>  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:

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

> ---
>  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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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
>>

On Fri, Nov 21, 2025 at 02:07:59PM +0800, 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>

Looks good, feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Regards,
ojaswin

> ---
>  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
>