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 ? Thanks, Yi. > Otherwise feel free to add: > > Reviewed-by: Jan Kara <jack@xxxxxxx> > > 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 >>
