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

[KunWu Chan]

Hi Alex,

> 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.
>

Thanks for confirming. That makes sense.

>
>
> >
> > 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 :)
>

A debug counter for sync devices would be useful to quantify how often
 the fallback actually happens.
I'll also take a look at the patchset Matthew and Barry pointed to.

Thanks,
 Kunwu


> Thanks for your comment!
>
> Alex
>
> >
> > Thanks,
> > KunWu
> >