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 5/9] ext4: make ext4_es_insert_delayed_block() insert multi-blocks
Date: Mon, 29 Apr 2024 18:27:25 +0200 [thread overview]
Message-ID: <20240429162725.rzj43hscw6to7xed@quack3> (raw)
In-Reply-To: <cf125f2c-d2f0-57f8-ee6f-9a93b9f5828d@huaweicloud.com>
On Mon 29-04-24 20:09:46, Zhang Yi wrote:
> On 2024/4/29 17:16, Jan Kara wrote:
> > On Wed 10-04-24 11:41:59, Zhang Yi wrote:
> >> From: Zhang Yi <yi.zhang@huawei.com>
> >>
> >> Rename ext4_es_insert_delayed_block() to ext4_es_insert_delayed_extent()
> >> and pass length parameter to make it insert multi delalloc blocks once a
> >> time. For the case of bigalloc, expand the allocated parameter to
> >> lclu_allocated and end_allocated. lclu_allocated indicates the allocate
> >> state of the cluster which containing the lblk, end_allocated represents
> >> the end, and the middle clusters must be unallocated.
> >>
> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
...
> >> @@ -2112,13 +2124,22 @@ void ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
> >> es2 = NULL;
> >> }
> >>
> >> - if (allocated) {
> >> - err3 = __insert_pending(inode, lblk, &pr);
> >> + if (lclu_allocated) {
> >> + err3 = __insert_pending(inode, lblk, &pr1);
> >> if (err3 != 0)
> >> goto error;
> >> - if (pr) {
> >> - __free_pending(pr);
> >> - pr = NULL;
> >> + if (pr1) {
> >> + __free_pending(pr1);
> >> + pr1 = NULL;
> >> + }
> >> + }
> >> + if (end_allocated) {
> >
> > So there's one unclear thing here: What if 'lblk' and 'end' are in the same
> > cluster? We don't want two pending reservation structures for the cluster.
> > __insert_pending() already handles this gracefully but perhaps we don't
> > need to allocate 'pr2' in that case and call __insert_pending() at all? I
> > think it could be easily handled by something like:
> >
> > if (EXT4_B2C(lblk) == EXT4_B2C(end))
> > end_allocated = false;
> >
> > at appropriate place in ext4_es_insert_delayed_extent().
> >
>
> I've done the check "EXT4_B2C(lblk) == EXT4_B2C(end)" in the caller
> ext4_insert_delayed_blocks() in patch 8, becasue there is no need to check
> the allocation state if they are in the same cluster, so it could make sure
> that end_allocated is always false when 'lblk' and 'end' are in the same
> cluster. So I suppose check and set it here again maybe redundant, how about
> add a wanging here in ext4_es_insert_delayed_extent(), like:
>
> WARN_ON_ONCE((EXT4_B2C(sbi, lblk) == EXT4_B2C(sbi, end)) &&
> end_allocated);
>
> and modify the 'lclu_allocated/end_allocated' parameter comments, note that
> end_allocated should always be set to false if the extent is in one cluster.
> Is it fine?
Yes, that is a good solution as well!
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2024-04-29 16:27 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
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 [this message]
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=20240429162725.rzj43hscw6to7xed@quack3 \
--to=jack@suse.cz \
--cc=adilger.kernel@dilger.ca \
--cc=chengzhihao1@huawei.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=yi.zhang@huawei.com \
--cc=yi.zhang@huaweicloud.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).