Received-SPF: pass (google.com: domain of linux-ext4+bounces-2189-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: Thu, 25 Apr 2024 17:56:40 +0200
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,
	yi.zhang@huawei.com, chengzhihao1@huawei.com, yukuai3@huawei.com
Subject: Re: [PATCH v2 3/9] ext4: trim delalloc extent
Message-ID: <20240425155640.ktvqqwhteitysaby@quack3>
References: <20240410034203.2188357-1-yi.zhang@huaweicloud.com>
 <20240410034203.2188357-4-yi.zhang@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240410034203.2188357-4-yi.zhang@huaweicloud.com>

On Wed 10-04-24 11:41:57, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> The cached delalloc or hole extent should be trimed to the map->map_len
> if we map delalloc blocks in ext4_da_map_blocks(). But it doesn't
> trigger any issue now because the map->m_len is always set to one and we
> always insert one delayed block once a time. Fix this by trim the extent
> once we get one from the cached extent tree, prearing for mapping a
> extent with multiple delalloc blocks.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Well, but we already do the trimming in ext4_da_map_blocks(), don't we? You
just move it to a different place... Or do you mean that we actually didn't
set 'map' at all in some cases and now we do? In either case the 'map'
handling looks a bit sloppy in ext4_da_map_blocks() as e.g. the
'add_delayed' case doesn't seem to bother with properly setting 'map' based
on what it does. So maybe we should clean that up to always set 'map' just
before returning at the same place where we update the 'bh'? And maybe bh
update could be updated in some common helper because it's content is
determined by the 'map' content?

								Honza

> ---
>  fs/ext4/inode.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 118b0497a954..e4043ddb07a5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1734,6 +1734,11 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>  
>  	/* Lookup extent status tree firstly */
>  	if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) {
> +		retval = es.es_len - (iblock - es.es_lblk);
> +		if (retval > map->m_len)
> +			retval = map->m_len;
> +		map->m_len = retval;
> +
>  		if (ext4_es_is_hole(&es))
>  			goto add_delayed;
>  
> @@ -1750,10 +1755,6 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>  		}
>  
>  		map->m_pblk = ext4_es_pblock(&es) + iblock - es.es_lblk;
> -		retval = es.es_len - (iblock - es.es_lblk);
> -		if (retval > map->m_len)
> -			retval = map->m_len;
> -		map->m_len = retval;
>  		if (ext4_es_is_written(&es))
>  			map->m_flags |= EXT4_MAP_MAPPED;
>  		else if (ext4_es_is_unwritten(&es))
> @@ -1788,6 +1789,11 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>  	 * whitout holding i_rwsem and folio lock.
>  	 */
>  	if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) {
> +		retval = es.es_len - (iblock - es.es_lblk);
> +		if (retval > map->m_len)
> +			retval = map->m_len;
> +		map->m_len = retval;
> +
>  		if (!ext4_es_is_hole(&es)) {
>  			up_write(&EXT4_I(inode)->i_data_sem);
>  			goto found;
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR