On Wed 07-08-24 20:18:18, Zhang Yi wrote:

On 2024/8/6 23:23, Jan Kara wrote:

quoted

On Fri 02-08-24 19:51:13, Zhang Yi wrote:

quoted

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
-- 
Jan Kara [off-list ref]
SUSE Labs, CR