Re: [PATCH v2 3/9] ext4: trim delalloc extent

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

On 2024/4/25 23:56, Jan Kara wrote:
> On Wed 10-04-24 11:41:57, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> The cached delalloc or hole extent should be trimed to the map->map_len
>> if we map delalloc blocks in ext4_da_map_blocks(). But it doesn't
>> trigger any issue now because the map->m_len is always set to one and we
>> always insert one delayed block once a time. Fix this by trim the extent
>> once we get one from the cached extent tree, prearing for mapping a
>> extent with multiple delalloc blocks.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> 
> Well, but we already do the trimming in ext4_da_map_blocks(), don't we? You
> just move it to a different place... Or do you mean that we actually didn't
> set 'map' at all in some cases and now we do? 

Yeah, now we only trim map len if we found an unwritten extent or written
extent in the cache, this isn't okay if we found a hole and
ext4_insert_delayed_block() and ext4_da_map_blocks() support inserting
map->len blocks. If we found a hole which es->es_len is shorter than the
length we want to write, we could delay more blocks than we expected.

Please assume we write data [A, C) to a file that contains a hole extent
[A, B) and a written extent [B, D) in cache.

                      A     B  C  D
before da write:   ...hhhhhh|wwwwww....

Then we will get extent [A, B), we should trim map->m_len to B-A before
inserting new delalloc blocks, if not, the range [B, C) is duplicated.

> In either case the 'map'
> handling looks a bit sloppy in ext4_da_map_blocks() as e.g. the
> 'add_delayed' case doesn't seem to bother with properly setting 'map' based
> on what it does. So maybe we should clean that up to always set 'map' just
> before returning at the same place where we update the 'bh'? And maybe bh
> update could be updated in some common helper because it's content is
> determined by the 'map' content?
> 

I agree with you, it looks that we should always revise the map->m_len
once we found an extent from the cache, and then do corresponding handling
according to the extent type. so it's hard to put it to a common place.
But we can merge the handling of written and unwritten extent, I've moved
the bh updating into ext4_da_get_block_prep() and do some cleanup in
patch 9, please look at that patch, does it looks fine to you?

Thanks,
Yi.

> 								Honza
> 
>> ---
>>  fs/ext4/inode.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 118b0497a954..e4043ddb07a5 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -1734,6 +1734,11 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>>  
>>  	/* Lookup extent status tree firstly */
>>  	if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) {
>> +		retval = es.es_len - (iblock - es.es_lblk);
>> +		if (retval > map->m_len)
>> +			retval = map->m_len;
>> +		map->m_len = retval;
>> +
>>  		if (ext4_es_is_hole(&es))
>>  			goto add_delayed;
>>  
>> @@ -1750,10 +1755,6 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>>  		}
>>  
>>  		map->m_pblk = ext4_es_pblock(&es) + iblock - es.es_lblk;
>> -		retval = es.es_len - (iblock - es.es_lblk);
>> -		if (retval > map->m_len)
>> -			retval = map->m_len;
>> -		map->m_len = retval;
>>  		if (ext4_es_is_written(&es))
>>  			map->m_flags |= EXT4_MAP_MAPPED;
>>  		else if (ext4_es_is_unwritten(&es))
>> @@ -1788,6 +1789,11 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>>  	 * whitout holding i_rwsem and folio lock.
>>  	 */
>>  	if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) {
>> +		retval = es.es_len - (iblock - es.es_lblk);
>> +		if (retval > map->m_len)
>> +			retval = map->m_len;
>> +		map->m_len = retval;
>> +
>>  		if (!ext4_es_is_hole(&es)) {
>>  			up_write(&EXT4_I(inode)->i_data_sem);
>>  			goto found;
>> -- 
>> 2.39.2
>>