Re: [PATCH 05/46] mm/truncate: look again if a folio left the mapping

[Kiryl Shutsemau]

On Thu, Oct 29, 2026 at 04:21:54PM +0100, David Hildenbrand (Arm) wrote:
> On 10/29/26 15:38, Kiryl Shutsemau wrote:
> > On Thu, Oct 29, 2026 at 03:18:23PM +0100, David Hildenbrand (Arm) wrote:
> >> On 10/29/26 12:37, Kiryl Shutsemau wrote:
> >>>
> >>> Yes, I missed that the loop only goes back to start when a lookup
> >>> comes back empty below end. When the last batch reaches end, it exits
> >>> without another look. It doesn't need the split either: any punch hole
> >>> whose last page is in the page cache gets there.
> >>>
> >>> That exit looks unintended to me. b9a8a4195c7d ("truncate,shmem: Handle
> >>> truncates that split large folios") turned the "if (start >= end) goto
> >>> out" early exit into the loop condition. Before it, the loop went back
> >>> to start until a lookup from start found nothing; Hugh kept that on
> >>> purpose for hole punch in 792ceaefe621 ("mm/fs: fix pessimization in
> >>> hole-punching pagecache").
> >>>
> >>> Restoring it is enough, and makes the "restart to make sure all gone"
> >>> comment true again:
> >>>
> >>> +       if (start >= end)
> >>> +               return;
> >>> +
> >>>         index = start;
> >>> -       while (index < end) {
> >>> +       for (;;) {
> >>>
> >>> Then a replaced folio is found on the next round, and truncate needs no
> >>> special case for it.
> >>>
> >>
> >> But that's then another lookup even if not really required?
> >>
> >> Why do you thing that is that better?
> >>
> >> I agree that the code is simpler, but at least it's consistent with the other
> >> handling.
> > 
> > The extra lookup only happens when the second loop had work to do.
> > Normally the first pass removes everything, the first lookup from start
> > comes back empty, and both versions stop there. When the second loop did
> > run to end, it costs one more lookup over a range that is now empty.
> > 
> > What I like about it is one way to look again instead of two: the loop
> > already goes back to start when the last batch ends below end, and the
> > patch adds a second way for when it ends at end. truncate is not like
> > shmem here: shmem_undo_range() stops after one sweep on hole punch on
> > purpose, so it needs the rewind; truncate was meant to sweep until
> > nothing is left.
> > 
> > Your version fixes the bug too, so I'll leave the choice to you.
> > 
> 
> Can we just do:

LGTM!

-- 
  Kiryl Shutsemau / Kirill A. Shutemov