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@xxxxxxxxxx> > >> > >> 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@xxxxxxxxxx> > > > > 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 <jack@xxxxxxxx> SUSE Labs, CR
