Re: [PATCH 1/2] mm: zswap: free synchronous-IO writeback folios directly

[Alexandre Ghiti]

Hi Nhat,

On Mon, Jul 20, 2026 at 5:56 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> >
> On Sat, Jul 18, 2026 at 2:38 AM Alexandre Ghiti <alex@xxxxxxxx> wrote:
> >
> > When zswap writes an entry back, it allocates a swap cache folio,
> > decompresses the entry into it and writes it out. That folio is cold by
> > construction, but it is currently left on the LRU for page reclaim to find
> > and free later. This wastes a reclaim scan and keeps cold memory resident
> > longer than necessary.
> >
> > For synchronous-IO swap devices writeback completes in the calling context,
> > so the folio can be freed right after the write rather than left behind; do
> > that. Because it is freed directly rather than through reclaim, it is
> > allocated off the LRU: dropping the last reference on a folio still on the
> > LRU would trip the free-time page-flag checks. A folio that a concurrent
> > swapin has meanwhile claimed is left in place and reclaimed as usual.
> >
> > Asynchronous and filesystem-backed swap complete writeback in interrupt
> > context, where the folio cannot be freed; they are handled in a later
> > change.
> >
> > Signed-off-by: Alexandre Ghiti <alex@xxxxxxxx>
> > ---
> >  mm/swap.h       |  3 ++-
> >  mm/swap_state.c | 17 +++++++++++------
> >  mm/zswap.c      | 36 ++++++++++++++++++++++++++++++++++--
> >  3 files changed, 47 insertions(+), 9 deletions(-)
> >
> > diff --git a/mm/swap.h b/mm/swap.h
> > index 77d2d14eda42..c617e5a0257f 100644
> > --- a/mm/swap.h
> > +++ b/mm/swap.h
> > @@ -306,7 +306,8 @@ void *swap_cache_get_shadow(swp_entry_t entry);
> >  void swap_cache_del_folio(struct folio *folio);
> >  struct folio *swap_cache_alloc_folio(swp_entry_t target_entry, gfp_t gfp_mask,
> >                                      unsigned long orders, struct vm_fault *vmf,
> > -                                    struct mempolicy *mpol, pgoff_t ilx);
> > +                                    struct mempolicy *mpol, pgoff_t ilx,
> > +                                    bool skip_lru);
> >  /* Below helpers require the caller to lock and pass in the swap cluster. */
> >  void __swap_cache_add_folio(struct swap_cluster_info *ci,
> >                             struct folio *folio, swp_entry_t entry);
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index 9c3a5cf99778..048efc7ca612 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -403,7 +403,8 @@ void __swap_cache_replace_folio(struct swap_cluster_info *ci,
> >  static struct folio *__swap_cache_alloc(struct swap_cluster_info *ci,
> >                                         swp_entry_t targ_entry, gfp_t gfp,
> >                                         unsigned int order, struct vm_fault *vmf,
> > -                                       struct mempolicy *mpol, pgoff_t ilx)
> > +                                       struct mempolicy *mpol, pgoff_t ilx,
> > +                                       bool skip_lru)
> >  {
> >         int err;
> >         swp_entry_t entry;
> > @@ -484,7 +485,8 @@ static struct folio *__swap_cache_alloc(struct swap_cluster_info *ci,
> >         lruvec_stat_mod_folio(folio, NR_SWAPCACHE, nr_pages);
> >
> >         /* Caller will initiate read into locked new_folio */
> > -       folio_add_lru(folio);
> > +       if (!skip_lru)
> > +               folio_add_lru(folio);
> >         return folio;
> >  }
> >
> > @@ -507,7 +509,8 @@ static struct folio *__swap_cache_alloc(struct swap_cluster_info *ci,
> >   */
> >  struct folio *swap_cache_alloc_folio(swp_entry_t targ_entry, gfp_t gfp,
> >                                      unsigned long orders, struct vm_fault *vmf,
> > -                                    struct mempolicy *mpol, pgoff_t ilx)
> > +                                    struct mempolicy *mpol, pgoff_t ilx,
> > +                                    bool skip_lru)
> >  {
> >         int order, err;
> >         struct folio *ret;
> > @@ -522,7 +525,7 @@ struct folio *swap_cache_alloc_folio(swp_entry_t targ_entry, gfp_t gfp,
> >
> >         do {
> >                 ret = __swap_cache_alloc(ci, targ_entry, gfp, order,
> > -                                        vmf, mpol, ilx);
> > +                                        vmf, mpol, ilx, skip_lru);
> >                 if (!IS_ERR(ret))
> >                         break;
> >                 err = PTR_ERR(ret);
> > @@ -643,7 +646,8 @@ static struct folio *swap_cache_read_folio(swp_entry_t entry, gfp_t gfp,
> >                 folio = swap_cache_get_folio(entry);
> >                 if (folio)
> >                         return folio;
> > -               folio = swap_cache_alloc_folio(entry, gfp, BIT(0), NULL, mpol, ilx);
> > +               folio = swap_cache_alloc_folio(entry, gfp, BIT(0), NULL, mpol, ilx,
> > +                                              false);
> >         } while (PTR_ERR(folio) == -EEXIST);
> >
> >         if (IS_ERR_OR_NULL(folio))
> > @@ -683,7 +687,8 @@ struct folio *swapin_sync(swp_entry_t entry, gfp_t gfp, unsigned long orders,
> >                 folio = swap_cache_get_folio(entry);
> >                 if (folio)
> >                         return folio;
> > -               folio = swap_cache_alloc_folio(entry, gfp, orders, vmf, mpol, ilx);
> > +               folio = swap_cache_alloc_folio(entry, gfp, orders, vmf, mpol, ilx,
> > +                                              false);
> >         } while (PTR_ERR(folio) == -EEXIST);
> >
> >         if (IS_ERR(folio))
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 761cd699e0a3..3c494c5a671c 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -972,6 +972,30 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> >  /*********************************
> >  * writeback code
> >  **********************************/
> > +static void zswap_writeback_free_folio(struct folio *folio)
> > +{
> > +       folio_lock(folio);
> > +
> > +       /* The folio was allocated off the LRU and nothing re-adds it here. */
> > +       VM_WARN_ON_ONCE_FOLIO(folio_test_lru(folio), folio);
> > +
> > +       /*
> > +        * Gate remove_mapping() on folio_test_swapcache(): a racing swapin may
> > +        * have freed the swap slot (folio_free_swap()) and dropped the folio from
> > +        * the cache, and remove_mapping() must not run on a non-swapcache folio
> > +        * (it would trip __remove_mapping()'s mapping == folio_mapping() check).
> > +        */
> > +       if (folio_test_swapcache(folio) &&
> > +           remove_mapping(swap_address_space(folio->swap), folio))
>
> Hmm I think you also have to test for writeback and dirty here right?
>
> After a racing swapin freed the swap slot, the folio might be dirtied
> and swapped out again. We probably shouldn't try to remove_mapping()
> such a folio?
>
> We also do similar check for zswap/sync-io devices in pageout handling
> (mm/vmscan.c, shrink_folio_list()), and the other dropbehind handler
> (filemap_end_dropbehind()).

Actually it took me some time to eliminate the writeback check, I
think it is not necessary.

First, we only call remove_mapping() at the end of writeback (easy scenario).

Then comes your scenario, the hard one. But it cannot happen because
dropbehind folios are allocated off lru. Which means that a racing
swapin can re-dirty the folio (that will get caught by remove_mapping
itself) but it cannot be swapped out (ie under writeback) because it
is off lru and then cannot be reclaimed.

But writing that, I realize I may have missed other "swap triggers"
(MADV_PAGEOUT for example). Perhaps adding the check is safer (and
cheap anyway), what do you think?

>
> When I was playing with this, I also tested the folio with
> folio_test_active() and folio_mapped(), but I must admit - I do not
> remember why I did it. Could just be me making sure we dont get rid of
> frequently used folio rather than for strict correctness. :)
>

folio_mapped() is not necessary because remove_mapping() checks the
refcount. folio_test_active() is always false in my case since
allocated off-lru.

So unless someone confirms what I said above for writeback, I'll check
for it, it's cheap anyway.

Thanks,

Alex