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

[Alexandre Ghiti]

On Fri, Aug 21, 2026 at 11:53 AM Alexandre Ghiti <alexghiti@xxxxxxxx> wrote:
>
> 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 :)
>

As you noted, in my implementation, a race was possible between the
worker and the folio_put() in zswap_writeback_entry(). Now with Tal's
patchset which *only* defers atomic context to a workqueue, the
dropbehind folios on synchronous devices get freed inline which is not
possible because of the reference held by the zswap writeback path.

But it's cleanly fixable by dropping the zswap reference before
__swap_writepage(): the folio is locked (when it is allocated) until
writeback starts, then PG_writeback prevents the folio removal from
the swapcache and right before clearing PG_writeback in
folio_end_writeback_no_dropbehind(), folio_end_writeback() takes a
reference! So remove_mapping() is happy with the refcount :)

So there are no more races as you pointed out, since we drop the
reference before the writeback. I'll post the v4 today.

Thanks again for your comment.

Alex



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