Re: [PATCH v2 1/4] ext4: remove useless code in ext4_map_create_blocks

[yangerkun <yangerkun@huawei.com>]

在 2025/11/4 22:28, Jan Kara 写道:
> On Tue 04-11-25 21:17:47, 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.
>>
>> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
>> Signed-off-by: Yang Erkun <yangerkun@huawei.com>
> 
> I agree the check isn't needed for correctness but it seems to be
> reasonable performance optimization for a common case of writing back
> already written data in dioread_nolock mode?

Hi!

Thank you for your detailed review! I believe you are referring to
writing back a block within the written extent in dioread_nolock mode.
If that's the case, we might never enter ext4_map_create_blocks because
ext4_map_query_blocks will return the block as MAPPED. Please correct me
if I misunderstood!

Thanks,
Erkun.

> 
> 								Honza
> 
>> ---
>>   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,
>> -- 
>> 2.39.2
>>