Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 24 May 2022 11:56:55 +0530
From:   Ritesh Harjani <ritesh.list@gmail.com>
To:     Jan Kara <jack@suse.cz>
Cc:     Baokun Li <libaokun1@huawei.com>, linux-ext4@vger.kernel.org,
        tytso@mit.edu, adilger.kernel@dilger.ca,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
        yebin10@huawei.com, yukuai3@huawei.com
Subject: Re: [PATCH 2/2] ext4: correct the judgment of BUG in
 ext4_mb_normalize_request
Message-ID: <20220524062655.ddiltnfxxhlelfgb@riteshh-domain>
References: <20220521134217.312071-1-libaokun1@huawei.com>
 <20220521134217.312071-3-libaokun1@huawei.com>
 <20220523200844.fal3pmp7epid3rvv@riteshh-domain>
 <20220523210806.yeapg54ctleocwdn@quack3.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220523210806.yeapg54ctleocwdn@quack3.lan>
Precedence: bulk

On 22/05/23 11:08PM, Jan Kara wrote:
> On Tue 24-05-22 01:38:44, Ritesh Harjani wrote:
> > On 22/05/21 09:42PM, Baokun Li wrote:
> > > When either of the "start + size <= ac->ac_o_ex.fe_logical" or
> > > "start > ac->ac_o_ex.fe_logical" conditions is met, it indicates
> > > that the fe_logical is not in the allocated range.
> >
> > Sounds about right to me based on the logic in ext4_mb_use_inode_pa().
> > We try to allocate/preallocate such that ac->ac_o_ex.fe_logical should fall
> > within the preallocated range. So if our start or start + size doesn't include
> > fe_logical then it is a bug in the ext4_mb_normalize_request() logic.
>
> I agree ac->ac_o_ex.fe_logical is a goal block. But AFAIK it never was a
> hard guarantee that we would allocate extent that includes that block. It

Agree that the guarantee is not about the extent which finally gets allocated.
It is only within ext4_mb_normalize_request() that the "start" and "size"
variable calculations is done in such a way that the ac->ac_o_ex.fe_logical
block will always fall within the "start" & "end" boundaries after
normalization.

That is how it also updates the goal block at the end too. ac->ac_g_ex.

> was always treated as a hint only. In particular if you look at the logic
> in ext4_mb_normalize_request() it does shift the start of the allocation to
> avoid preallocated ranges etc.

Yes, I checked the logic of ext4_mb_normalize_request() again.
As I see it (I can be wrong, so please correct me), there is always an attempt
to make "start" & "start + size" such that it covers ac->ac_o_ex.fe_logical
except just one change where we are trimming the size of the request to only
EXT4_BLOCKS_PER_GROUP.

For e.g. when it compares against preallocated ranges. It has a BUG() which
checks if the ac->ac_o_ex.fe_logical already lies in the preallocated range.
Because then we should never have come here to do allocation of a new block.

4143                 /* PA must not overlap original request */
4144                 BUG_ON(!(ac->ac_o_ex.fe_logical >= pa_end ||
4145                         ac->ac_o_ex.fe_logical < pa->pa_lstart));
<...>
4152                 BUG_ON(pa->pa_lstart <= start && pa_end >= end);

Then after skipping the preallocated regions which doesn't fall in between
"start" and "end"...

4147                 /* skip PAs this normalized request doesn't overlap with */
4148                 if (pa->pa_lstart >= end || pa_end <= start) {
4149                         spin_unlock(&pa->pa_lock);
4150                         continue;
4151                 }

...it adjusts "start" and "end" boundary according to allocated PAs boundaries
such that fe_logical is always in between "start" and "end".

4154                 /* adjust start or end to be adjacent to this pa */
4155                 if (pa_end <= ac->ac_o_ex.fe_logical) {
4156                         BUG_ON(pa_end < start);
4157                         start = pa_end;
4158                 } else if (pa->pa_lstart > ac->ac_o_ex.fe_logical) {
4159                         BUG_ON(pa->pa_lstart > end);
4160                         end = pa->pa_lstart;
4161                 }


> so I don't see how we are guaranteed that
> ext4_mb_normalize_request() will result in an allocation request that
> includes ac->ac_o_ex.fe_logical.

It could be I am wrong, but looks like ext4_mb_normalize_request() keeps
ac->ac_o_ex.fe_logical within "start" and "end" of allocation request.
And then updates the goal block.

4196         ac->ac_g_ex.fe_logical = start;
4197         ac->ac_g_ex.fe_len = EXT4_NUM_B2C(sbi, size);

Thoughts?

-ritesh