Received-SPF: pass (google.com: domain of linux-ext4+bounces-2230-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 29 Apr 2024 12:25:50 +0200
From: Jan Kara <jack@suse.cz>
To: Zhang Yi <yi.zhang@huaweicloud.com>
Cc: Jan Kara <jack@suse.cz>, linux-ext4@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, tytso@mit.edu,
	adilger.kernel@dilger.ca, yi.zhang@huawei.com,
	chengzhihao1@huawei.com, yukuai3@huawei.com
Subject: Re: [PATCH v2 3/9] ext4: trim delalloc extent
Message-ID: <20240429102550.sx4vdl75whxovmc2@quack3>
References: <20240410034203.2188357-1-yi.zhang@huaweicloud.com>
 <20240410034203.2188357-4-yi.zhang@huaweicloud.com>
 <20240425155640.ktvqqwhteitysaby@quack3>
 <acd4e7c9-c68b-9edc-bba4-dce5e8ce7879@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <acd4e7c9-c68b-9edc-bba4-dce5e8ce7879@huaweicloud.com>

On Fri 26-04-24 17:38:23, Zhang Yi wrote:
> On 2024/4/25 23:56, Jan Kara wrote:
> > 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? 
> 
> Yeah, now we only trim map len if we found an unwritten extent or written
> extent in the cache, this isn't okay if we found a hole and
> ext4_insert_delayed_block() and ext4_da_map_blocks() support inserting
> map->len blocks. If we found a hole which es->es_len is shorter than the
> length we want to write, we could delay more blocks than we expected.
> 
> Please assume we write data [A, C) to a file that contains a hole extent
> [A, B) and a written extent [B, D) in cache.
> 
>                       A     B  C  D
> before da write:   ...hhhhhh|wwwwww....
> 
> Then we will get extent [A, B), we should trim map->m_len to B-A before
> inserting new delalloc blocks, if not, the range [B, C) is duplicated.

Thanks for explanation!

> > 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?
> > 
> 
> I agree with you, it looks that we should always revise the map->m_len
> once we found an extent from the cache, and then do corresponding handling
> according to the extent type. so it's hard to put it to a common place.
> But we can merge the handling of written and unwritten extent, I've moved
> the bh updating into ext4_da_get_block_prep() and do some cleanup in
> patch 9, please look at that patch, does it looks fine to you?

Oh, yes, what patch 9 does improve things significantly and it addresses my
concern. So feel free to add:

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

Maybe in the changelog you can just mention that the remaining cases not
setting map->m_len will be handled in patch 9.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR