linux-kernel - Re: [PATCH v2 02/10] ext4: optimize the EXT4_GET_BLOCKS_DELALLOC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240806144818.rt2vb677cxghxykz@quack3>
Date: Tue, 6 Aug 2024 16:48:18 +0200
From: Jan Kara <jack@...e.cz>
To: Zhang Yi <yi.zhang@...weicloud.com>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, tytso@....edu,
	adilger.kernel@...ger.ca, jack@...e.cz, ritesh.list@...il.com,
	yi.zhang@...wei.com, chengzhihao1@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH v2 02/10] ext4: optimize the
 EXT4_GET_BLOCKS_DELALLOC_RESERVE flag set

On Fri 02-08-24 19:51:12, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@...wei.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@...wei.com>

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

Reviewed-by: Jan Kara <jack@...e.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@...e.com>
SUSE Labs, CR