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

[Kiryl Shutsemau]

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:

  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?

> @@ -760,7 +768,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
> 

-- 
  Kiryl Shutsemau / Kirill A. Shutemov