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

[Kunwu Chan <kunwu.chan@gmail.com>]

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?

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.

Thanks,
KunWu