Messages in this thread | | | Date | Tue, 24 May 2022 11:56:55 +0530 | From | Ritesh Harjani <> | Subject | Re: [PATCH 2/2] ext4: correct the judgment of BUG in ext4_mb_normalize_request |
| |
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
| |