Re: [PATCH v3 3/3] mm: zswap: drop cold writeback folios via swap dropbehind

[Alexandre Ghiti]

I am pasting and answering Sashiko's review inline below.

On Tue, Aug 18, 2026 at 6:35 PM Alexandre Ghiti <alex@xxxxxxxx> wrote:
>
> >
> zswap writeback decompresses an entry into a fresh swap cache folio and
> writes it back. The folio is cold by construction, yet it is left on the
> LRU for reclaim to find and free later, wasting a reclaim scan and keeping
> cold memory resident longer than necessary.
>
> Allocate the folio off the LRU and mark it PG_dropbehind so the swap
> dropbehind path frees it from the swap cache once writeback completes.
>
> Suggested-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> Suggested-by: Nhat Pham <nphamcs@xxxxxxxxx>
> Signed-off-by: Alexandre Ghiti <alex@xxxxxxxx>
> ---
>  mm/zswap.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 8163e6c5f76c..a35671e837f8 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1013,7 +1013,6 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>          */
>         if (IS_ERR(folio))
>                 return PTR_ERR(folio);
> -       folio_add_lru(folio);
>
>         /*
>          * folio is locked, and the swapcache is now secured against
> @@ -1046,8 +1045,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>         /* folio is up to date */
>         folio_mark_uptodate(folio);
>
> -       /* move it to the tail of the inactive list after end_writeback */
> -       folio_set_reclaim(folio);
> +       folio_set_dropbehind(folio);

Sashiko: "When setting dropbehind here, what happens if a concurrent swapoff
(try_to_unuse) removes the folio from the swap cache before the dropbehind
worker runs?
Looking at swap_dropbehind_drop_folio() in mm/swap_state.c:
if (folio_test_swapcache(folio) && !folio_test_writeback(folio) &&
remove_mapping(swap_address_space(folio->swap), folio, true, memcg)) {
dropped = true;
} else {
/* Raced: the folio is now owned by the swapin; put it back. */
folio_clear_dropbehind(folio);
folio_add_lru(folio);
}
If try_to_unuse() removes the folio from the swap cache, folio_test_swapcache()
will be false. The dropbehind worker will then blindly add the unmapped
anonymous folio to the LRU.
When reclaimed later, shrink_folio_list() will see it has PG_swapbacked but
no swap cache, causing it to allocate a new swap entry, mark it dirty, and
needlessly write the data back out to disk.
Could this race cause ghost anonymous folios to be added to the LRU?"

If swapoff frees the swap slot while the folio is queued for the dropbehind
drop, then yes, the worker takes the fallback and puts the folio back on the
LRU and that is exactly what has to happen: swapoff has just invalidated the
on-disk copy, so the folio now holds the only copy of the data. Leaving it off
the LRU would make it unreclaimable. Note the folio is marked dirty by
folio_free_swap() itself, for the same reason.

This is also not new: in the current code zswap's writeback folio is already on
the LRU when try_to_unuse() calls folio_free_swap() on it, so the resulting
state (unmapped, dirty, anon, on the LRU, no swap entry) is the same with and
without this series. Only the caller of folio_add_lru() differs.

>
>         /* start writeback */
>         __swap_writepage(folio, NULL);

Sashiko: "Does holding a reference to the folio until after
__swap_writepage() returns
prevent the dropbehind optimization from working on fast or synchronous swap
devices?
Because zswap_writeback_entry() holds a reference to the folio that isn't
released until it calls folio_put() at the end of the function, a race
can occur with synchronous I/O.
For synchronous devices (like zram), folio_end_writeback() is called and the
dropbehind worker is queued before __swap_writepage() even returns.
If the dropbehind worker calls remove_mapping() before zswap_writeback_entry()
calls folio_put(), remove_mapping() will see an unexpectedly high refcount
and fail.
The worker will then falsely assume a racing swapin and add the cold folio to
the LRU, defeating the purpose of the patch."

Yes, that race is real but I estimated it would be rare enough since
the folio_put() call is right after __swap_writepage(), and the time
the worker wakes up, the window is very small to me. I haven't
measured the actual rate, but I can measure it if needed. And if the
race is rare with sync devices, it is even more unlikely with async
devices.

In the end, I don't think any modifications are needed based on
Sashiko's review, let me know what you think.

Thanks,

Alex

> --
> 2.53.0-Meta
>
>