diff mbox series

[v2,03/10] ext4: don't set EXTENT_STATUS_DELAYED on allocated blocks

Message ID 20240802115120.362902-4-yi.zhang@huaweicloud.com (mailing list archive)
State New
Headers show
Series ext4: simplify the counting and management of delalloc reserved blocks | expand

Commit Message

Zhang Yi Aug. 2, 2024, 11:51 a.m. UTC 
From: Zhang Yi <yi.zhang@huawei.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@huawei.com>
---
 fs/ext4/extents_status.c |  9 +--------
 fs/ext4/inode.c          | 11 -----------
 2 files changed, 1 insertion(+), 19 deletions(-)

Comments

Jan Kara Aug. 6, 2024, 3:23 p.m. UTC | #1 
On Fri 02-08-24 19:51:13, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.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@huawei.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.

Otherwise feel free to add:

Reviewed-by: Jan Kara <jack@suse.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
>
Zhang Yi Aug. 7, 2024, 12:18 p.m. UTC | #2 
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@huawei.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@huawei.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@suse.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
>>
Jan Kara Aug. 7, 2024, 1:37 p.m. UTC | #3 
On Wed 07-08-24 20:18:18, Zhang Yi wrote:
> 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@huawei.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@huawei.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 ?

Now you've got me confused again ;) Why I wrote the changelog that way is
because ext4_es_remove_extent() is calling ext4_da_release_space(). But now
I've realized I've confused ext4_es_remove_extent() with
__es_remove_extent() which is what gets called when inserting another
extent. So I was wrong and indeed your version of the last sentense is
correct. Thanks for catching this!

								Honza
diff mbox series

Patch

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 17dcf13adde2..024cd37d53b3 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -869,14 +869,7 @@  void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 		return;
 
 	BUG_ON(end < lblk);
-
-	if ((status & EXTENT_STATUS_DELAYED) &&
-	    (status & EXTENT_STATUS_WRITTEN)) {
-		ext4_warning(inode->i_sb, "Inserting extent [%u/%u] as "
-				" delayed and written which can potentially "
-				" cause data loss.", lblk, len);
-		WARN_ON(1);
-	}
+	WARN_ON_ONCE(status & EXTENT_STATUS_DELAYED);
 
 	newes.es_lblk = lblk;
 	newes.es_len = len;
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);
 	}