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

[KunWu Chan]

On Mon, Aug 24, 2026 at 5:30 PM Alexandre Ghiti <alexghiti@xxxxxxxx> wrote:
>
> 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 :)
> >
>

Hi Alex,

Thanks for checking this and for the detailed explanation.

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

That makes sense.
Dropping the zswap reference before __swap_writepage()  avoids the
overlap, while PG_writeback prevents the folio from being removed  from
the swap cache until writeback completion.
The reference taken before clearing PG_writeback then ensures that
remove_mapping() sees the expected refcount.

I agree this is cleaner than bringing back the synchronous-I/O special case.

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

I'll take a look at v4 after it is posted.

Thanks,
KunWu

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