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

[David Hildenbrand (Arm)]

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:

diff --git a/mm/truncate.c b/mm/truncate.c
index 733d2713a875..9e9bf029635c 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -475,8 +475,11 @@ void truncate_inode_pages_range(struct address_space *mapping,
 		}
 	}
 
+	if (start >= end)
+		return;
+
 	index = start;
-	while (index < end) {
+	while (true) {
 		cond_resched();
 		if (!find_get_entries(mapping, &index, end - 1, &fbatch,
 				indices)) {
@@ -499,6 +502,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
 			folio_lock(folio);
 			VM_BUG_ON_FOLIO(!folio_contains(folio, indices[i]), folio);
 			folio_wait_writeback(folio);
+			/* Do not handle errors, we'll do another lookup */
 			truncate_inode_folio(mapping, folio);
 			folio_unlock(folio);
 		}
@@ -760,7 +764,12 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
 			folio_lock(folio);
 			if (unlikely(folio->mapping != mapping)) {
 				folio_unlock(folio);
-				continue;
+				/*
+				 * Folio might have been replaced: look up
+				 * again.
+				 */
+				index = indices[i];
+				break;
 			}
 			VM_BUG_ON_FOLIO(!folio_contains(folio, indices[i]), folio);
 			folio_wait_writeback(folio);
-- 
2.43.0


That would be easier for me to digest than the index <= end thing.


-- 
Cheers,

David