Received-SPF: pass (google.com: domain of linux-ext4+bounces-656-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 3 Jan 2024 12:02:55 +0100
From: Jan Kara <jack@suse.cz>
To: Zhang Yi <yi.zhang@huaweicloud.com>
Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz,
	ritesh.list@gmail.com, hch@infradead.org, djwong@kernel.org,
	willy@infradead.org, yi.zhang@huawei.com, chengzhihao1@huawei.com,
	yukuai3@huawei.com, wangkefeng.wang@huawei.com
Subject: Re: [RFC PATCH v2 03/25] ext4: correct the hole length returned by
 ext4_map_blocks()
Message-ID: <20240103110255.7uebqb2iy4jcy6sh@quack3>
References: <20240102123918.799062-1-yi.zhang@huaweicloud.com>
 <20240102123918.799062-4-yi.zhang@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240102123918.799062-4-yi.zhang@huaweicloud.com>

On Tue 02-01-24 20:38:56, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> In ext4_map_blocks(), if we can't find a range of mapping in the
> extents cache, we are calling ext4_ext_map_blocks() to search the real
> path and ext4_ext_determine_hole() to determine the hole range. But if
> the querying range was partially or completely overlaped by a delalloc
> extent, we can't find it in the real extent path, so the returned hole
> length could be incorrect.
> 
> Fortunately, ext4_ext_put_gap_in_cache() have already handle delalloc
> extent, but it searches start from the expanded hole_start, doesn't
> start from the querying range, so the delalloc extent found could not be
> the one that overlaped the querying range, plus, it also didn't adjust
> the hole length. Let's just remove ext4_ext_put_gap_in_cache(), handle
> delalloc and insert adjusted hole extent in ext4_ext_determine_hole().
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Suggested-by: Jan Kara <jack@suse.cz>

Some small comments below.

> @@ -4062,6 +4038,66 @@ static int get_implied_cluster_alloc(struct super_block *sb,
>  	return 0;
>  }
>  
> +/*
> + * Determine hole length around the given logical block, first try to
> + * locate and expand the hole from the given @path, and then adjust it
> + * if it's partially or completely converted to delayed extents.
> + */
> +static void ext4_ext_determine_hole(struct inode *inode,
> +				    struct ext4_ext_path *path,
> +				    struct ext4_map_blocks *map)
> +{

I'd prefer to keep setting of 'map' inside ext4_ext_map_blocks() to make it
more obvious there and just pass map->m_lblk into
ext4_ext_determine_hole(). ext4_ext_determine_hole() will just return the
length of the extent from lblk that was mapped (i.e. 'len').

Also I'd probably call this function like ext4_ext_determine_insert_hole()
to make it more obvious the function modifies the status tree.

Otherwise the patch looks good to me.

								Honza

> +	ext4_lblk_t hole_start, len;
> +	struct extent_status es;
> +
> +	hole_start = map->m_lblk;
> +	len = ext4_ext_find_hole(inode, path, &hole_start);
> +again:
> +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, hole_start,
> +				  hole_start + len - 1, &es);
> +	if (!es.es_len)
> +		goto insert_hole;
> +
> +	/*
> +	 * Found a delayed range in the hole, handle it if the delayed
> +	 * range is before, behind and straddle the queried range.
> +	 */
> +	if (map->m_lblk >= es.es_lblk + es.es_len) {
> +		/*
> +		 * Before the queried range, find again from the queried
> +		 * start block.
> +		 */
> +		len -= map->m_lblk - hole_start;
> +		hole_start = map->m_lblk;
> +		goto again;
> +	} else if (in_range(map->m_lblk, es.es_lblk, es.es_len)) {
> +		/*
> +		 * Straddle the beginning of the queried range, it's no
> +		 * longer a hole, adjust the length to the delayed extent's
> +		 * after map->m_lblk.
> +		 */
> +		len = es.es_lblk + es.es_len - map->m_lblk;
> +		goto out;
> +	} else {
> +		/*
> +		 * Partially or completely behind the queried range, update
> +		 * hole length until the beginning of the delayed extent.
> +		 */
> +		len = min(es.es_lblk - hole_start, len);
> +	}
> +
> +insert_hole:
> +	/* Put just found gap into cache to speed up subsequent requests */
> +	ext_debug(inode, " -> %u:%u\n", hole_start, len);
> +	ext4_es_insert_extent(inode, hole_start, len, ~0, EXTENT_STATUS_HOLE);
> +
> +	/* Update hole_len to reflect hole size after map->m_lblk */
> +	if (hole_start != map->m_lblk)
> +		len -= map->m_lblk - hole_start;
> +out:
> +	map->m_pblk = 0;
> +	map->m_len = min_t(unsigned int, map->m_len, len);
> +}
>  
>  /*
>   * Block allocation/map/preallocation routine for extents based files
> @@ -4179,22 +4215,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>  	 * we couldn't try to create block if create flag is zero
>  	 */
>  	if ((flags & EXT4_GET_BLOCKS_CREATE) == 0) {
> -		ext4_lblk_t hole_start, hole_len;
> -
> -		hole_start = map->m_lblk;
> -		hole_len = ext4_ext_determine_hole(inode, path, &hole_start);
> -		/*
> -		 * put just found gap into cache to speed up
> -		 * subsequent requests
> -		 */
> -		ext4_ext_put_gap_in_cache(inode, hole_start, hole_len);
> -
> -		/* Update hole_len to reflect hole size after map->m_lblk */
> -		if (hole_start != map->m_lblk)
> -			hole_len -= map->m_lblk - hole_start;
> -		map->m_pblk = 0;
> -		map->m_len = min_t(unsigned int, map->m_len, hole_len);
> -
> +		ext4_ext_determine_hole(inode, path, map);
>  		goto out;
>  	}
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR