On 2024/2/1 19:08, Ojaswin Mujoo wrote: Hi Ojaswin, Jan
Hi Baokun, Jan Thanks for the CC, I somehow missed this patch. As described in the discussion Jan linked [1] , there is a known bug in the normalize code (which i should probably get back to now ) where we sometimes end up with a goal range which doesn't completely cover the original extent and this was causing issues when we tried to cover the complete original request in the PA window adjustment logic. That and to minimize fragmentation, we ended up going with the logic we have right now. In short, I agree that in the example Baokun pointed out, it is not optimal to have to make an allocation request twice when we can get it in one go. I also think Baokun is correct that if keeping the best extent at the end doesn't cover the original start, then any other case should not lead to it overflowing out of goal extent, including the case where original extent is overflowing goal extent. So, as mentioned, it boils down to a trade off between multiple allocations and slightly increased fragmentation. iiuc preallocations are anyways dropped when the file closes so I think it shouldn't hurt too much fragmentation wise to prioritize less allocations. What are your thoughts on this Jan, Baokun? Coming to the code, the only thing I think might cause an issue is the following line: - BUG_ON(ac->ac_o_ex.fe_len > ac->ac_b_ex.fe_len); + BUG_ON(o_ex_end > extent_logical_end(sbi, &ex)); So as discussed towards the end here [1] we could have ac_o_ex that overflows the goal and hence would be beyond the best length. I'll try to look into the normalize logic to fix this however till then, I think we should not have this BUG_ON since it would crash the kernel if this happens. Rest of it looks good to me. Regards, Ojaswin [1] https://lore.kernel.org/all/Y+UzQJRIJEiAr4Z4@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
I will remove the problematic BUG_ON and add some comments in the next version. Thanks to Ojaswin and Jan for the review! -- With Best Regards, Baokun Li .
