Re: [PATCH mm-new v1 2/2] mm: vmscan: skip anon folios zswap would refuse when writeback is off

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm: vmscan: unmapped swapcache folios bypass swap_writeout_blocked()
- [Medium] mm: vmscan: large and split head folios bypass swap_writeout_blocked()
--

commit 6d6f21b971f8e3a837e970c969d5f1fdbeb97cbe
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

mm: vmscan: skip anon folios zswap would refuse when writeback is off

This commit adds swap_writeout_blocked() and zswap_store_blocked() to detect
when a memcg with memory.zswap.writeback=0 cannot store pages in zswap. In
shrink_folio_list(), non-zero-filled anonymous folios are activated early
before allocating swap slots and unmapping them when swap writeout is
blocked.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 7ddef3b488b3d..591a98cd9ef59 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1169,6 +1169,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>  	unsigned int nr_reclaimed = 0, nr_demoted = 0;
>  	unsigned int pgactivate = 0;
>  	bool do_demote_pass;
> +	int swap_blocked = -1;
>  	struct swap_io_ctx ctx = {};
>  
>  	folio_batch_init(&free_folios);
> @@ -1355,6 +1356,22 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>  			continue;
>  		}
>  
> +		/*
> +		 * swap_writeout() would only activate this folio. Do that now,
> +		 * before it costs a swap slot and an unmap. Zero-filled folios
> +		 * are marked in the swap table without a store, an unmapped one
> +		 * may only need its stale slot dropped and a large one may still
> +		 * be split, let them go on.
> +		 */
> +		if (folio_test_anon(folio) && folio_test_swapbacked(folio) &&
> +		    folio_mapped(folio) && !folio_test_large(folio) &&
> +		    (!folio_test_swapcache(folio) || folio_test_dirty(folio))) {
> +			if (swap_blocked < 0)
> +				swap_blocked = swap_writeout_blocked(memcg);
> +			if (swap_blocked && !is_folio_zero_filled(folio))
> +				goto activate_locked;
> +		}

[Severity: Medium]
Does requiring folio_mapped(folio) allow unmapped dirty swap-cache folios
with active swap PTEs to bypass this check on subsequent reclaim passes?

When try_to_unmap() unmaps an anonymous folio during reclaim,
try_to_unmap_one() installs non-present swap PTEs via folio_dup_swap() and
removes the present PTEs. Once swap_writeout() rejects the folio with
AOP_WRITEPAGE_ACTIVATE (for example, when a clean swap-cache folio had a
dirty PTE settled by unmap, or when a folio was in the batch as zswap
became full), the folio stays in the swap cache marked dirty with
folio_mapped(folio) == false and folio_maybe_swapped(folio) == true.

If userspace does not fault the cold folio back in, folio_mapped(folio)
remains false on every subsequent pass through shrink_folio_list(), so the
folio skips this check and enters pageout() -> swap_writeout():

mm/page_io.c:swap_writeout() {
    if (folio_free_swap(folio))
        goto out_unlock;
    if (is_folio_zero_filled(folio)) {
        ...
    }
    swap_zeromap_folio_clear(folio);
    if (zswap_store(folio)) {
        ...
    }
    if (!mem_cgroup_zswap_writeback_enabled(folio_memcg(folio)))
        return AOP_WRITEPAGE_ACTIVATE;
}

Because folio_free_swap() checks folio_maybe_swapped(folio), it returns
false when swap PTEs still reference the slot:

mm/swapfile.c:folio_free_swap() {
    if (!folio_swapcache_freeable(folio))
        return false;
    if (folio_maybe_swapped(folio))
        return false;
    ...
}

Wouldn't those unmapped swap-cache folios repeatedly run
is_folio_zero_filled(), swap_zeromap_folio_clear(), and zswap_store() on
every reclaim cycle instead of being activated early?

[Severity: Medium]
How does !folio_test_large(folio) interact with CONFIG_THP_SWAP and with
the head folio left behind by split_folio_to_list()?

First, when CONFIG_THP_SWAP is enabled, a normal fully-mapped large
anonymous folio is not split before folio_alloc_swap():

mm/vmscan.c:shrink_folio_list() {
    if (folio_test_large(folio)) {
        if (folio_expected_ref_count(folio) !=
            folio_ref_count(folio) - 1)
            goto activate_locked;
        if (data_race(!list_empty(&folio->_deferred_list) &&
            folio_test_partially_mapped(folio)) &&
            split_folio_to_list(folio, folio_list))
            goto activate_locked;
    }
    ret = folio_alloc_swap(folio);
}

When folio_alloc_swap() succeeds for the large folio without returning
-E2BIG, the entire large folio is unmapped by try_to_unmap() and rejected
in swap_writeout() with AOP_WRITEPAGE_ACTIVATE, and remains a large folio
on subsequent passes.

Second, even when split_folio_to_list(folio, folio_list) splits a large
folio (either for a partially-mapped folio on _deferred_list or when
folio_alloc_swap() returns -E2BIG), only the tail pages are queued onto
folio_list to re-enter the loop. The head folio (now order-0) continues
directly to folio_alloc_swap(folio) and try_to_unmap() in the current
iteration without passing through the swap_writeout_blocked() check.

Should large folios and split head folios also be checked against
swap_writeout_blocked() before allocating swap and unmapping?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030095223.325309-1-jiayuan.chen@linux.dev?part=2