Re: [PATCH v3 2/3] mm: swap: drop dropbehind swap cache folios on writeback completion

[Alexandre Ghiti]

Hi Kunwu,

On Fri, Aug 21, 2026 at 10:35 AM Kunwu Chan <kunwu.chan@xxxxxxxxx> wrote:
>
> >
>
> Hi Alexandre,
>
> I have a question about the reference ownership/lifetime model in the
> synchronous-IO case.
>
> Consider the synchronous writeback path in Patch 3:
>
>   zswap_writeback_entry()
>     folio = __swap_cache_alloc_folio(...);
>     ...
>     __swap_writepage(folio, NULL);
>     folio_put(folio);
>
> During writeback completion, Patch 2 does:
>
>   folio_end_writeback(folio)
>     folio_get(folio);
>     ...
>     swap_writeback_dropbehind_folio(folio);
>       llist_add()
>       queue_work()
>
> With synchronous I/O, is it possible for the dropbehind worker to run
> before zswap_writeback_entry() drops its reference after
> __swap_writepage() returns?

Yes, totally! The window is short though and in that rare case, we
would fall back to the initial behaviour (almost, since we put the
folio back onto lru but not rotate it). And for async devices, I don't
think it can happen at all.

>
> If so, the worker will attempt:
>
>   swap_dropbehind_drop_folio(folio)
>     ...
>     remove_mapping(swap_address_space(folio->swap), folio, true, memcg)
>
> while the caller's reference is still held.  Looking at
> __remove_mapping() in mm/vmscan.c, it expects a refcount of
> 1 + folio_nr_pages(folio), but the extra reference still held by
> zswap_writeback_entry() would make the actual refcount one higher.
> It therefore looks like folio_ref_freeze() will fail in this case.
> Is that the intended behavior here?
>
> If the removal can fail, the fallback is:
>
>     folio_clear_dropbehind(folio);
>     folio_add_lru(folio);
>
> This appears semantically safe, but it means that the dropbehind
> optimization is lost for that writeback: the cold folio goes back onto
> the LRU and has to be found by reclaim later.
>
> This seems particularly worth checking because v3 intentionally removes
> the synchronous-I/O special case from v2.  The cover letter describes
> that special case as an optimization that was not worth the extra code,
> which I agree is a reasonable direction if the generic path is
> sufficiently effective.
>
> Could you measure the corresponding fallback rate on a synchronous
> backend (e.g. zram)?  The 99.996% success rate in the cover letter is for
> asynchronous NVMe, so it does not tell us how often this particular race
> occurs with synchronous completion.
>
> More generally, I'd like the reference ownership across
> __swap_writepage(), folio_end_writeback(), and the deferred worker to be
> made explicit.  If the caller's reference can overlap with the worker's
> reference, I'd also like to understand whether that is an intentional
> and acceptable trade-off, or whether the overlap can be avoided without
> reintroducing the synchronous-I/O special case that v3 is trying to
> remove.

I need to look into the patchset that Matthew and Barry pointed to and
check if it's still relevant with this patchset on top of it. If so,
I'll add a debug counter to confirm we don't fall back too much for
sync devices :)

Thanks for your comment!

Alex

>
> Thanks,
> KunWu
>