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

[David Hildenbrand (Arm)]

On 10/28/26 17:54, Kiryl Shutsemau wrote:
> On Tue, Oct 27, 2026 at 12:17:25AM +0100, David Hildenbrand (Arm) wrote:
>> invalidate_inode_pages2_range() skips a folio which no longer belongs to
>> the mapping by the time it has got the folio lock, on the assumption
>> that somebody else has removed it from the page cache. But the folio may
>> have been replaced, and then there is a new folio with the same data at
>> that index.
>>
>> For most cases, like folio migration, that is impossible. However,
>> collapse_file() only checks the refcount of the old folios, under the
>> i_pages lock, and relies on their folio locks from there on. That works for
>> users which repeat the lookup when they find folio->mapping changed.
>> The following scenario is, however, possible:
>>
>>   CPU0 pwrite(O_DIRECT)               CPU1 collapse_file()
>>   kiocb_invalidate_pages()
>>                                       read the folio (old data), lock it
>>                                       refcount check
>>   the write completes                 copy to new_folio
>>   kiocb_invalidate_post_direct_write()
>>     find_get_entries() -> old folio
>>     folio_lock() ...                  xas_store(new_folio)
>>                                       folio->mapping = NULL
>>                                       folio_unlock(folio)
>>     folio->mapping != mapping
>>       -> continue
> 
> The diagram reads as if the folio lock should have closed the race. It
> might be clearer with CPU0's folio_lock() after CPU1's folio_unlock(),
> since the reference taken after the refcount check is what matters:

I'll use that, thanks!

> 
>   CPU0 pwrite(O_DIRECT)               CPU1 collapse_file()
>   kiocb_invalidate_pages()
>                                       read the folio (old data), lock it
>                                       refcount check
>   the write completes                 copy to new_folio
>   kiocb_invalidate_post_direct_write()
>     find_get_entries() -> old folio
>                                       xas_store(new_folio)
>                                       folio->mapping = NULL
>                                       folio_unlock(folio)
>     folio_lock()
>     folio->mapping != mapping
>       -> continue
> 
>> In that scenario, the collapsed folio with the data from before the write
>> stays in the page cache after the write has returned.
>>
>> This used to be impossible, as khugepaged only collapsed files that nobody
>> had open for writing.
>>
>> Restart the lookup at that index, like shmem_undo_range() does. The same
>> problem seems to exist in truncate_inode_pages_range() as well when
>> truncate_inode_folio() fails, so handle it there as well.
>>
>> Fixes: 1b6444331beb ("mm/khugepaged: enable clean pagecache folio collapse for writable files")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Reported-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>> ---
>>  mm/truncate.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/truncate.c b/mm/truncate.c
>> index 733d2713a875a..6d8c0ec4623fe 100644
>> --- a/mm/truncate.c
>> +++ b/mm/truncate.c
>> @@ -499,7 +499,15 @@ 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);
>> -			truncate_inode_folio(mapping, folio);
>> +			if (truncate_inode_folio(mapping, folio)) {
>> +				folio_unlock(folio);
>> +				/*
>> +				 * Folio might have been replaced: look it up
>> +				 * again.
>> +				 */
>> +				index = indices[i];
>> +				break;
>> +			}
>>  			folio_unlock(folio);
>>  		}
>>  		truncate_folio_batch_exceptionals(mapping, &fbatch, indices);
> 
> This hunk looks unneeded to me: its second loop restarts from start
> until find_get_entries() finds nothing, so the new folio gets truncated
> on a later round. Or am I missing a case?
For truncate_inode_pages_range() I *think* the "index >= end" might prevent
another lookup that we would need.

Assume we have a large folio with 512 pages for simplicity, and we want to
punch-hole the first half of it.

The whole partial splitting logic (truncate_inode_partial_folio) will first
split that folio buddy-like into various folios.

Now we want to remove the split folios in the loop:

index = 0;
end = 256;
while (index < 256)
	if (!find_get_entries(mapping, &index, 255, &fbatch, ...

	-> Returns our split folios from the buddy split, the last one should
           cover pages 128 to 255
	-> Sets index = 256

Then we have the concurrent stuff happening. We wouldn't re-execute the
find_get_entries() even though a folio was replaced in the meantime, as index =256.

My brain hurts, does the above make sense?

-- 
Cheers,

David