From: Zhang Yi <yi.zhang@huaweicloud.com>
To: Jan Kara <jack@suse.cz>
Cc: 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
Date: Mon, 29 Apr 2024 19:28:56 +0800 [thread overview]
Message-ID: <9d4d958b-7b70-c8f5-e091-174a99f506b6@huaweicloud.com> (raw)
In-Reply-To: <20240429102550.sx4vdl75whxovmc2@quack3>
On 2024/4/29 18:25, Jan Kara wrote:
> 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!
>
Current change log is not clear enough now, I will put this explanation
into the changelog in my nextn iteratio, make it easier to understand.
>>> 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.
>
Sure.
Thanks,
Yi.
next prev parent reply other threads:[~2024-04-29 11:29 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-10 3:41 [PATCH v2 0/9] ext4: support adding multi-delalloc blocks Zhang Yi
2024-04-10 3:41 ` [PATCH v2 1/9] ext4: factor out a common helper to query extent map Zhang Yi
2024-04-24 20:05 ` Jan Kara
2024-04-10 3:41 ` [PATCH v2 2/9] ext4: check the extent status again before inserting delalloc block Zhang Yi
2024-04-24 20:05 ` Jan Kara
2024-04-10 3:41 ` [PATCH v2 3/9] ext4: trim delalloc extent Zhang Yi
2024-04-25 15:56 ` Jan Kara
2024-04-26 9:38 ` Zhang Yi
2024-04-29 10:25 ` Jan Kara
2024-04-29 11:28 ` Zhang Yi [this message]
2024-04-10 3:41 ` [PATCH v2 4/9] ext4: drop iblock parameter Zhang Yi
2024-04-29 7:34 ` Jan Kara
2024-04-10 3:41 ` [PATCH v2 5/9] ext4: make ext4_es_insert_delayed_block() insert multi-blocks Zhang Yi
2024-04-29 9:16 ` Jan Kara
2024-04-29 12:09 ` Zhang Yi
2024-04-29 16:27 ` Jan Kara
2024-04-29 9:24 ` Jan Kara
2024-04-10 3:42 ` [PATCH v2 6/9] ext4: make ext4_da_reserve_space() reserve multi-clusters Zhang Yi
2024-04-29 9:25 ` Jan Kara
2024-04-10 3:42 ` [PATCH v2 7/9] ext4: factor out check for whether a cluster is allocated Zhang Yi
2024-04-10 3:42 ` [PATCH v2 8/9] ext4: make ext4_insert_delayed_block() insert multi-blocks Zhang Yi
2024-04-29 10:06 ` Jan Kara
2024-04-29 12:54 ` Zhang Yi
2024-04-10 3:42 ` [PATCH v2 9/9] ext4: make ext4_da_map_blocks() buffer_head unaware Zhang Yi
2024-04-29 10:27 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9d4d958b-7b70-c8f5-e091-174a99f506b6@huaweicloud.com \
--to=yi.zhang@huaweicloud.com \
--cc=adilger.kernel@dilger.ca \
--cc=chengzhihao1@huawei.com \
--cc=jack@suse.cz \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=yi.zhang@huawei.com \
--cc=yukuai3@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.