linux-ext4 - Re: [PATCH 1/4] ext4: remove useless code in ext4_map_create

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: <ed114762-fe34-458d-989f-e0fbf3afd42a@huaweicloud.com>
Date: Thu, 30 Oct 2025 21:12:25 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Yang Erkun <yangerkun@...wei.com>, linux-ext4@...r.kernel.org
Cc: libaokun1@...wei.com, yangerkun@...weicloud.com, tytso@....edu,
 adilger.kernel@...ger.ca, jack@...e.cz
Subject: Re: [PATCH 1/4] ext4: remove useless code in ext4_map_create_blocks

On 10/27/2025 8:23 PM, Yang Erkun wrote:
> IO path with EXT4_GET_BLOCKS_PRE_IO means dio within i_size or
> dioread_nolock buffer writeback, they all means we need a unwritten
> extent(or this extent has already been initialized), and the split won't
> zero the range we really write. So this check seems useless. Besides,
> even if we repeatedly execute ext4_es_insert_extent, there won't
> actually be any issues.
> 
> Signed-off-by: Yang Erkun <yangerkun@...wei.com>

Before submitting I/O with EXT4_GET_BLOCKS_PRE_IO flag set, if
ext4_split_extent_at() tries to split a large unwritten extent but fails
to due to ENOSPC, it will zero out the leftover part but still leave the
origin large extent intact. However, it will insert the zeroed part into
the extent status tree as written. This write zeros and inserting
operations seems useless because:
1. After the I/O is completed, ext4_convert_unwritten_extents_endio()
   will call ext4_split_convert_extents() to the same thing again.
2. If I/O fails, there will be no stale data issue because the extent
   remains unwritten.

Leaving the written extent in the status tree would also lead to
inconsistencies with the state in the actual tree. Although this won't
cause any practical issues. So...

Reviewed-by: Zhang Yi <yi.zhang@...wei.com>

> ---
>  fs/ext4/inode.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index e99306a8f47c..e8bac93ca668 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -583,7 +583,6 @@ static int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
>  static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
>  				  struct ext4_map_blocks *map, int flags)
>  {
> -	struct extent_status es;
>  	unsigned int status;
>  	int err, retval = 0;
>  
> @@ -644,16 +643,6 @@ static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
>  			return err;
>  	}
>  
> -	/*
> -	 * If the extent has been zeroed out, we don't need to update
> -	 * extent status tree.
> -	 */
> -	if (flags & EXT4_GET_BLOCKS_PRE_IO &&
> -	    ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) {
> -		if (ext4_es_is_written(&es))
> -			return retval;
> -	}
> -
>  	status = map->m_flags & EXT4_MAP_UNWRITTEN ?
>  			EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>  	ext4_es_insert_extent(inode, map->m_lblk, map->m_len, map->m_pblk,