Re: [PATCH v6 06/12] mm, swap: write back vswap zswap entries to physical swap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm: zswap: corrupted swap accounting and shadow on vswap writeback failure
--

commit 790470c2c322c0c4b7b975b139e54f69bf472f15
Author: Nhat Pham <nphamcs@gmail.com>

mm, swap: write back vswap zswap entries to physical swap

This commit adds support for writing back zswap-backed vswap entries to
physical swap by allocating physical swap slots on demand during writeback.
It also updates the zswap shrinker to only skip vswap entries when no
physical swap space is available.

> diff --git a/mm/zswap.c b/mm/zswap.c
> index 1b91dba992aba..fd0c2f126b570 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1076,12 +1076,13 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>  static int zswap_writeback_entry(struct zswap_entry *entry,
>  				 swp_entry_t swpentry)
>  {
> -	struct xarray *tree;
>  	pgoff_t offset = swp_offset(swpentry);
>  	struct folio *folio;
>  	struct mempolicy *mpol;
>  	struct swap_info_struct *si;
>  	struct swap_io_ctx ctx = {};
> +	swp_entry_t phys = {};
> +	bool is_vswap;
>  	int ret = 0;
>  
>  	/* try to allocate swap cache folio */
> @@ -1089,12 +1090,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>  	if (IS_ERR_OR_NULL(si))
>  		return -ENOENT;
>  
> -	/* Vswap entries have no physical backing to write to. */
> -	if (swap_is_vswap(si)) {
> -		put_swap_device(si);
> -		return -EINVAL;
> -	}
> -
> +	is_vswap = swap_is_vswap(si);
>  	mpol = get_task_policy(current);
>  	folio = __swap_cache_alloc_folio(swpentry, GFP_KERNEL, BIT(0), NULL, mpol,
>  					 NO_INTERLEAVE_INDEX);
> @@ -1115,24 +1111,44 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>  	/*
>  	 * folio is locked, and the swapcache is now secured against
>  	 * concurrent swapping to and from the slot, and concurrent
> -	 * swapoff so we can safely dereference the zswap tree here.
> +	 * swapoff so we can safely dereference the zswap tree (or vswap
> +	 * vtable) here.
>  	 * Verify that the swap entry hasn't been invalidated and recycled
>  	 * behind our backs, to avoid overwriting a new swap folio with
>  	 * old compressed data. Only when this is successful can the entry
>  	 * be dereferenced.
>  	 */
> -	tree = swap_zswap_tree(swpentry);
> -	if (entry != xa_load(tree, offset)) {
> +	if (entry != zswap_entry_load(swpentry)) {
>  		ret = -ENOMEM;
>  		goto err;
>  	}
>  
> +	if (is_vswap) {
> +		/*
> +		 * Allocate physical backing before decompress so a failure
> +		 * wastes no work.
> +		 */
> +		phys = folio_realloc_swap(folio);
> +		if (!phys.val) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +	}

[Severity: Medium]
What happens to the swap_table shadow and cgroup v1 swap accounting when
folio_realloc_swap() fails here and jumps to err?

Earlier in zswap_writeback_entry(), __swap_cache_alloc_folio() calls
__swap_cache_alloc(), which consumes the existing workingset shadow and
memcg_id from the cluster, replaces the swap_table slot with the folio, and
runs memcg1_swapin() and workingset_refault():

mm/swap_state.c:__swap_cache_alloc() {
	...
	ci = swap_cluster_lock(si, offset);
	if (ci)
		err = __swap_cache_add_check(ci, targ_entry, nr_pages, &shadow, &memcg_id);
	...
	__swap_cache_do_add_folio(ci, folio, entry);
	swap_cluster_unlock(ci);
	...
	/* memsw uncharges swap when folio is added to swap cache */
	memcg1_swapin(folio);
	if (shadow)
		workingset_refault(folio, shadow);
	...
}

Under cgroup v1 (do_memsw_account()), memcg1_swapin() clears the slot's
memcg_table entry, uncharges memcg->memsw, and decrements MEMCG_SWAP and
MEMCG_VSWAP.

When folio_realloc_swap() returns !phys.val (for example, if physical swap
space or cgroup swap margin runs out mid-shrink), the error path in
zswap_writeback_entry() cleans up via swap_cache_del_folio() and folio_put():

err:
	swap_cache_del_folio(folio);
	folio_unlock(folio);
	folio_put(folio);

Because swap_cache_del_folio() calls __swap_cache_del_folio(ci, folio, entry,
NULL, false):

1. Passing NULL for shadow overwrites the swap_table slot with an empty
   shadow, losing the workingset eviction shadow for the entry that remains
   swapped out in zswap.
2. Passing false for swapout skips __memcg1_swapout(folio, ci), so when
   folio_put() frees the folio, __mem_cgroup_uncharge() uncharges
   memcg->memsw a second time while leaving ci->memcg_table cleared to 0
   and MEMCG_SWAP / MEMCG_VSWAP decremented for the still-live swap entry.
   A subsequent writeback attempt on that entry will see memcg_id == 0 and
   mm == NULL in mem_cgroup_swapin_charge_folio().

In addition, zswap_writeback_entry() returns -ENOMEM when !phys.val, which
shrink_memcg_cb() treats as LRU_RETRY:

mm/zswap.c:shrink_memcg_cb() {
	...
	writeback_result = zswap_writeback_entry(entry, swpentry);

	if (writeback_result) {
		zswap_reject_reclaim_fail++;
		ret = LRU_RETRY;
	...
}

If physical swap fills up during a shrink pass, won't the shrinker continue
scanning the rest of its budget (SWAP_CLUSTER_MAX per node in shrink_memcg()
or sc->nr_to_scan in zswap_shrinker_scan()), clearing entry->referenced on
referenced entries and repeating the __swap_cache_alloc_folio() and
swap_cache_del_folio() cycle on every unreferenced vswap entry?

[ ... ]

>
> -- 
> 2.51.0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021060323.553433-1-nphamcs@gmail.com?part=6