Re: [PATCH v2 02/10] ext4: optimize the EXT4_GET_BLOCKS_DELALLOC_RESERVE flag set

[Jan Kara <jack@suse.cz>]

On Fri 02-08-24 19:51:12, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> When doing block allocation, magic EXT4_GET_BLOCKS_DELALLOC_RESERVE
> means the allocating range covers a range of delayed allocated clusters,
> the blocks and quotas have already been reserved in ext4_da_map_blocks(),
> we should update the reserved space and don't need to claim them again.
> 
> At the moment, we only set this magic in mpage_map_one_extent() when
> allocating a range of delayed allocated clusters in the write back path,
> it makes things complicated since we have to notice and deal with the
> case of allocating non-delayed allocated clusters separately in
> ext4_ext_map_blocks(). For example, it we fallocate some blocks that
> have been delayed allocated, free space would be claimed again in
> ext4_mb_new_blocks() (this is wrong exactily), and we can't claim quota
> space again, we have to release the quota reservations made for that
> previously delayed allocated clusters.
> 
> Move the position thats set the EXT4_GET_BLOCKS_DELALLOC_RESERVE to
> where we actually do block allocation, it could simplify above handling
> a lot, it means that we always set this magic once the allocation range
> covers delalloc blocks, no need to take care of the allocation path.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Ah, nice idea. The patch looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/inode.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 112aec171ee9..91b2610a6dc5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -489,6 +489,14 @@ static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
>  	unsigned int status;
>  	int err, retval = 0;
>  
> +	/*
> +	 * We pass in the magic EXT4_GET_BLOCKS_DELALLOC_RESERVE
> +	 * indicates that the blocks and quotas has already been
> +	 * checked when the data was copied into the page cache.
> +	 */
> +	if (map->m_flags & EXT4_MAP_DELAYED)
> +		flags |= EXT4_GET_BLOCKS_DELALLOC_RESERVE;
> +
>  	/*
>  	 * Here we clear m_flags because after allocating an new extent,
>  	 * it will be set again.
> @@ -2224,11 +2232,6 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
>  	 * writeback and there is nothing we can do about it so it might result
>  	 * in data loss.  So use reserved blocks to allocate metadata if
>  	 * possible.
> -	 *
> -	 * We pass in the magic EXT4_GET_BLOCKS_DELALLOC_RESERVE if
> -	 * the blocks in question are delalloc blocks.  This indicates
> -	 * that the blocks and quotas has already been checked when
> -	 * the data was copied into the page cache.
>  	 */
>  	get_blocks_flags = EXT4_GET_BLOCKS_CREATE |
>  			   EXT4_GET_BLOCKS_METADATA_NOFAIL |
> @@ -2236,8 +2239,6 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
>  	dioread_nolock = ext4_should_dioread_nolock(inode);
>  	if (dioread_nolock)
>  		get_blocks_flags |= EXT4_GET_BLOCKS_IO_CREATE_EXT;
> -	if (map->m_flags & BIT(BH_Delay))
> -		get_blocks_flags |= EXT4_GET_BLOCKS_DELALLOC_RESERVE;
>  
>  	err = ext4_map_blocks(handle, inode, map, get_blocks_flags);
>  	if (err < 0)
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR