Re: [PATCH 06/16] mm/shmem: hold off eviction in shmem_writeout()

[Nhat Pham]

On Mon, Oct 26, 2026 at 8:14 AM Pedro Falcato <pfalcato@xxxxxxx> wrote:
>
> On Mon, Oct 26, 2026 at 10:40:39AM +0100, David Hildenbrand (Arm) wrote:
> > From: Josef Bacik <josef@xxxxxxxxxxxxxx>
> >
> > Once shmem_writeout() has moved the folio from the page cache to the
> > swap cache, the folio lock no longer keeps the inode alive: truncation
> > and eviction find a swap entry, free it and do not wait for anybody.
> > That did not matter as long as swap_writeout() was the last thing the
> > function did.  But since it puts the folio back when swap_writeout()
> > returns AOP_WRITEPAGE_ACTIVATE, it uses the mapping and the inode
> > afterwards:
> >
> >   reclaim                               last close of an unlinked file
> >   shmem_writeout()
> >     shmem_delete_from_page_cache()
> >     swap_writeout()
> >       zswap_store() fails               shmem_evict_inode()
> >       writeback is disabled               shmem_truncate_range()
> >       return AOP_WRITEPAGE_ACTIVATE         shmem_free_swap()
> >                                         destroy_inode()
> >                                         ... RCU grace period, freed
> >     shmem_add_to_page_cache(mapping)
> >       xas_lock_irq()            <- use after free
> >
> > swap_writeout() behaves that way if zswap is enabled, cannot take the
> > folio and the memcg or one of its ancestors has memory.zswap.writeback
> > set to 0.
> >
> > shmem_unuse() already added infrastructure to deal with that.
> > Reuse info->stop_eviction to protect the inode during writeout. Increment
> > it before removing the folio from the page cache, and decrement it after
> > swap_writeout() and any attempted reinsertion and inode accounting.
> > In shmem_evict_inode(), wait for the counter to reach zero even if
> > swapoff has already removed the inode from the swap list. Add some helpers
> > to make the code easier to digest.
> >
> > Fix generated by an LLM and sent by Josef Bacik in private.
> >
> > Fixes: 6344a6d9ce13 ("mm/shmem: writeout free swap if swap_writeout() reactivates")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > [ david: code cleanups; improve description ]
> > Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>
> I've been looking at this for a while trying to get a smoother fix, but:
>
> Acked-by: Pedro Falcato <pfalcato@xxxxxxx>
>
> Things I looked at:
> 1) Could we just drop this logic? No, it's fairly recent and seems to have
> impact on zswap setups with no writeback.
>
> 2) Could we reorder swap_writeout with removing the folio from the page cache?
> Perhaps, but it would be a larger change. I'm not sure if the logic all works out,
> but you could possibly have a:
>
> error = swap_writeout(...);
> if (error != AOP_WRITEPAGE_ACTIVATE) {
>         shmem_delete_from_page_cache(folio, swp_to_radix_entry(folio->swap));

Hmmm, say the backend is zswap. On successful zswap_store(), we drop
the folio's lock (in swap_writeout()) before returning.

Would it still be safe to delete from page cache with the folio
unlocked? :/ Seems dangerous - without the folio locked all bets are
off.

(analogously, filemap_remove_folio() even explicitly bug-out if
folio's lock is not held).

Maybe we there is line of reasoning to prove that this is safe that
I'm missing, but it just seems rather shaky/untested...