Re: [PATCH v2 01/10] ext4: factor out ext4_map_create_blocks() to allocate new blocks

[Jan Kara <jack@suse.cz>]

On Fri 02-08-24 19:51:11, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Factor out a common helper ext4_map_create_blocks() from
> ext4_map_blocks() to do a real blocks allocation, no logic changes.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks OK. Feel free to add:

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

								Honza

> ---
>  fs/ext4/inode.c | 157 +++++++++++++++++++++++++-----------------------
>  1 file changed, 81 insertions(+), 76 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 941c1c0d5c6e..112aec171ee9 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -482,6 +482,86 @@ static int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
>  	return retval;
>  }
>  
> +static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
> +				  struct ext4_map_blocks *map, int flags)
> +{
> +	struct extent_status es;
> +	unsigned int status;
> +	int err, retval = 0;
> +
> +	/*
> +	 * Here we clear m_flags because after allocating an new extent,
> +	 * it will be set again.
> +	 */
> +	map->m_flags &= ~EXT4_MAP_FLAGS;
> +
> +	/*
> +	 * We need to check for EXT4 here because migrate could have
> +	 * changed the inode type in between.
> +	 */
> +	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
> +		retval = ext4_ext_map_blocks(handle, inode, map, flags);
> +	} else {
> +		retval = ext4_ind_map_blocks(handle, inode, map, flags);
> +
> +		/*
> +		 * We allocated new blocks which will result in i_data's
> +		 * format changing. Force the migrate to fail by clearing
> +		 * migrate flags.
> +		 */
> +		if (retval > 0 && map->m_flags & EXT4_MAP_NEW)
> +			ext4_clear_inode_state(inode, EXT4_STATE_EXT_MIGRATE);
> +	}
> +	if (retval <= 0)
> +		return retval;
> +
> +	if (unlikely(retval != map->m_len)) {
> +		ext4_warning(inode->i_sb,
> +			     "ES len assertion failed for inode %lu: "
> +			     "retval %d != map->m_len %d",
> +			     inode->i_ino, retval, map->m_len);
> +		WARN_ON(1);
> +	}
> +
> +	/*
> +	 * We have to zeroout blocks before inserting them into extent
> +	 * status tree. Otherwise someone could look them up there and
> +	 * use them before they are really zeroed. We also have to
> +	 * unmap metadata before zeroing as otherwise writeback can
> +	 * overwrite zeros with stale data from block device.
> +	 */
> +	if (flags & EXT4_GET_BLOCKS_ZERO &&
> +	    map->m_flags & EXT4_MAP_MAPPED && map->m_flags & EXT4_MAP_NEW) {
> +		err = ext4_issue_zeroout(inode, map->m_lblk, map->m_pblk,
> +					 map->m_len);
> +		if (err)
> +			return err;
> +	}
> +
> +	/*
> +	 * If the extent has been zeroed out, we don't need to update
> +	 * extent status tree.
> +	 */
> +	if (flags & EXT4_GET_BLOCKS_PRE_IO &&
> +	    ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) {
> +		if (ext4_es_is_written(&es))
> +			return retval;
> +	}
> +
> +	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);
> +
> +	return retval;
> +}
> +
>  /*
>   * The ext4_map_blocks() function tries to look up the requested blocks,
>   * and returns if the blocks are already mapped.
> @@ -630,12 +710,6 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  		if (!(flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN))
>  			return retval;
>  
> -	/*
> -	 * Here we clear m_flags because after allocating an new extent,
> -	 * it will be set again.
> -	 */
> -	map->m_flags &= ~EXT4_MAP_FLAGS;
> -
>  	/*
>  	 * New blocks allocate and/or writing to unwritten extent
>  	 * will possibly result in updating i_data, so we take
> @@ -643,76 +717,7 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  	 * with create == 1 flag.
>  	 */
>  	down_write(&EXT4_I(inode)->i_data_sem);
> -
> -	/*
> -	 * We need to check for EXT4 here because migrate
> -	 * could have changed the inode type in between
> -	 */
> -	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
> -		retval = ext4_ext_map_blocks(handle, inode, map, flags);
> -	} else {
> -		retval = ext4_ind_map_blocks(handle, inode, map, flags);
> -
> -		if (retval > 0 && map->m_flags & EXT4_MAP_NEW) {
> -			/*
> -			 * We allocated new blocks which will result in
> -			 * i_data's format changing.  Force the migrate
> -			 * to fail by clearing migrate flags
> -			 */
> -			ext4_clear_inode_state(inode, EXT4_STATE_EXT_MIGRATE);
> -		}
> -	}
> -
> -	if (retval > 0) {
> -		unsigned int status;
> -
> -		if (unlikely(retval != map->m_len)) {
> -			ext4_warning(inode->i_sb,
> -				     "ES len assertion failed for inode "
> -				     "%lu: retval %d != map->m_len %d",
> -				     inode->i_ino, retval, map->m_len);
> -			WARN_ON(1);
> -		}
> -
> -		/*
> -		 * We have to zeroout blocks before inserting them into extent
> -		 * status tree. Otherwise someone could look them up there and
> -		 * use them before they are really zeroed. We also have to
> -		 * unmap metadata before zeroing as otherwise writeback can
> -		 * overwrite zeros with stale data from block device.
> -		 */
> -		if (flags & EXT4_GET_BLOCKS_ZERO &&
> -		    map->m_flags & EXT4_MAP_MAPPED &&
> -		    map->m_flags & EXT4_MAP_NEW) {
> -			ret = ext4_issue_zeroout(inode, map->m_lblk,
> -						 map->m_pblk, map->m_len);
> -			if (ret) {
> -				retval = ret;
> -				goto out_sem;
> -			}
> -		}
> -
> -		/*
> -		 * If the extent has been zeroed out, we don't need to update
> -		 * extent status tree.
> -		 */
> -		if ((flags & EXT4_GET_BLOCKS_PRE_IO) &&
> -		    ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) {
> -			if (ext4_es_is_written(&es))
> -				goto out_sem;
> -		}
> -		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);
> -	}
> -
> -out_sem:
> +	retval = ext4_map_create_blocks(handle, inode, map, flags);
>  	up_write((&EXT4_I(inode)->i_data_sem));
>  	if (retval > 0 && map->m_flags & EXT4_MAP_MAPPED) {
>  		ret = check_block_validity(inode, map);
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR