linux-ext4 - Re: [PATCH v2 03/10] ext4: don't set EXTENT_STATUS

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <685055bc-0d56-6cf3-7716-f27e448c8c38@huaweicloud.com>
Date: Wed, 7 Aug 2024 20:18:18 +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,
 ritesh.list@...il.com, yi.zhang@...wei.com, chengzhihao1@...wei.com,
 yukuai3@...wei.com
Subject: Re: [PATCH v2 03/10] ext4: don't set EXTENT_STATUS_DELAYED on
 allocated blocks

On 2024/8/6 23:23, Jan Kara wrote:
> On Fri 02-08-24 19:51:13, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@...wei.com>
>>
>> Since we always set EXT4_GET_BLOCKS_DELALLOC_RESERVE when allocating
>> delalloc blocks, there is no need to keep delayed flag on the unwritten
>> extent status entry, so just drop it after allocation.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> 
> Let me improve the changelog because I was confused for some time before I
> understood:
> 
> Currently, we release delayed allocation reservation when removing delayed
> extent from extent status tree (which also happens when overwriting one
> extent with another one). When we allocated unwritten extent under
> some delayed allocated extent, we don't need the reservation anymore and
> hence we don't need to preserve the EXT4_MAP_DELAYED status bit. Inserting
> the new extent into extent status tree will properly release the
> reservation.
> 

Thanks for your review and change log improvement. My original idea was very
simple, after patch 2, we always set EXT4_GET_BLOCKS_DELALLOC_RESERVE when
allocating blocks for delalloc extent, these two conditions in the 'if'
branch can never be true at the same time, so they become dead code and I
dropped them.

	if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) &&
	    ext4_es_scan_range(inode, &ext4_es_is_delayed, ...)

But after thinking your change log, I agree with you that we have already
properly update the reservation by searching delayed blocks through
ext4_es_delayed_clu() in ext4_ext_map_blocks() when we allocated unwritten
extent under some delayed allocated extent even it's not from the write
back path, so I think we can also drop them even without patch 2. But just
one point, I think the last last sentence isn't exactly true before path 6,
should it be "Allocating the new extent blocks will properly release the
reservation." now ?

Thanks,
Yi.

> Otherwise feel free to add:
> 
> Reviewed-by: Jan Kara <jack@...e.cz>
> 
> 								Honza
> 
> 
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 91b2610a6dc5..e9ce1e4e6acb 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -558,12 +558,6 @@ static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
>>  
>>  	status = map->m_flags & EXT4_MAP_UNWRITTEN ?
>>  			EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>> -	if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) &&
>> -	    !(status & EXTENT_STATUS_WRITTEN) &&
>> -	    ext4_es_scan_range(inode, &ext4_es_is_delayed, map->m_lblk,
>> -			       map->m_lblk + map->m_len - 1))
>> -		status |= EXTENT_STATUS_DELAYED;
>> -
>>  	ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
>>  			      map->m_pblk, status);
>>  
>> @@ -682,11 +676,6 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>>  
>>  		status = map->m_flags & EXT4_MAP_UNWRITTEN ?
>>  				EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>> -		if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) &&
>> -		    !(status & EXTENT_STATUS_WRITTEN) &&
>> -		    ext4_es_scan_range(inode, &ext4_es_is_delayed, map->m_lblk,
>> -				       map->m_lblk + map->m_len - 1))
>> -			status |= EXTENT_STATUS_DELAYED;
>>  		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
>>  				      map->m_pblk, status);
>>  	}
>> -- 
>> 2.39.2
>>