Re: [PATCH v5 01/19] mm: xswap support for zswap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mm/zswap: zswap_compress() stores incompressible SWP_XSWAP
  pages uncompressed
- [Medium] mm/swap_state: si shadowing and xswap neighbor readahead in
  swap_vma_readahead()
- [Medium] mm/zswap: infinite loop in shrink_worker() when
  mem_cgroup_disabled() with xswap
--

commit 5eef2108ac11c902b6d7f8f9358d479308d8aa73
Author: Chris Li <chrisl@kernel.org>

mm: xswap support for zswap

This commit adds extendable swap device (xswap) support for zswap,
keeping xswap entries only in zswap without backing storage, skipping
swap readahead and zswap LRU insertion for them, and gating zswap
writeback on active real swapfiles.

> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index da65d75ceead1..4c965cf99809b 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
[ ... ]
> @@ -984,6 +988,7 @@ static int swap_vma_ra_win(struct vm_fault *vmf, unsigned long *start,
>  static struct folio *swap_vma_readahead(swp_entry_t targ_entry, gfp_t gfp_mask,
>  		struct mempolicy *mpol, pgoff_t targ_ilx, struct vm_fault *vmf)
>  {
> +	struct swap_info_struct *si = __swap_entry_to_info(targ_entry);
>  	struct swap_io_ctx ctx = {};
>  	struct blk_plug plug;
>  	struct folio *folio;
> @@ -992,6 +997,10 @@ static struct folio *swap_vma_readahead(swp_entry_t targ_entry, gfp_t gfp_mask,
>  	unsigned long start, end, addr;
>  	pgoff_t ilx = targ_ilx;
>  
> +	/* xswap entries live only in zswap; readahead does not help. */
> +	if (si->flags & SWP_XSWAP)
> +		goto skip;
> +
>  	win = swap_vma_ra_win(vmf, &start, &end);
>  	if (win == 1)
>  		goto skip;

[Severity: Medium]
Can swap_vma_readahead() still perform readahead on neighboring SWP_XSWAP
entries when targ_entry is on a real swap device?

In the PTE loop below, neighboring entries in the VMA window may belong to
another swap device:

mm/swap_state.c:swap_vma_readahead() {
    ...
	for (addr = start; addr < end; ilx++, addr += PAGE_SIZE) {
		struct swap_info_struct *si = NULL;
    ...
		if (swp_type(entry) != swp_type(targ_entry)) {
			si = get_swap_device(entry);
			if (IS_ERR_OR_NULL(si))
				continue;
		}
		folio = swap_cache_read_folio(&ctx, entry, gfp_mask, mpol, ilx,
					      addr != vmf->address);
}

When a neighboring entry is on an SWP_XSWAP device, swap_cache_read_folio()
allocates a swapcache folio and calls zswap_load(), synchronously
decompressing and erasing the xswap entry from zswap during speculative
readahead.

Also, the new outer struct swap_info_struct *si declaration is shadowed by
the inner si declaration in the for loop, triggering a -Wshadow warning.

> diff --git a/mm/zswap.c b/mm/zswap.c
> index 37d217860a6b2..cef142d2ff856 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
[ ... ]
> @@ -1601,7 +1606,9 @@ static bool zswap_store_page(struct folio *folio, long index,
>  	entry->referenced = true;
>  	if (entry->length) {
>  		INIT_LIST_HEAD(&entry->lru);
> -		zswap_lru_add(entry);
> +		/* No backing store: nothing to write these back to. */
> +		if (!(__swap_entry_to_info(page_swpentry)->flags & SWP_XSWAP))
> +			zswap_lru_add(entry);
>  	}
>  
>  	return true;

[Severity: Medium]
Does zswap_compress() store incompressible SWP_XSWAP pages uncompressed at
PAGE_SIZE when cgroup zswap writeback is enabled?

mm/zswap.c:zswap_compress() {
    ...
	if (comp_ret) {
		rcu_read_lock();
		if (!mem_cgroup_zswap_writeback_enabled(folio_memcg(folio))) {
			rcu_read_unlock();
			goto unlock;
		}
		rcu_read_unlock();
		comp_ret = 0;
		dlen = PAGE_SIZE;
		dst = kmap_local_folio(folio, index * PAGE_SIZE);
		mapped = true;
	}
    ...
}

Storing PAGE_SIZE entries in zsmalloc exists only to preserve writeback LRU
order, but SWP_XSWAP entries are not added to zswap_list_lru and have no
backing store.  Accepting incompressible SWP_XSWAP pages wastes zswap pool
capacity and metadata on unreclaimable entries instead of failing
zswap_store() so swap_writeout() can reactivate the folio via
AOP_WRITEPAGE_ACTIVATE.  Should zswap_compress() also reject comp_ret != 0
when the folio's swap device has SWP_XSWAP set?

> @@ -1671,7 +1678,7 @@ bool zswap_store(struct folio *folio)
>  	zswap_pool_put(pool);
>  put_objcg:
>  	obj_cgroup_put(objcg);
> -	if (!ret && zswap_pool_reached_full)
> +	if (!ret && zswap_pool_reached_full && atomic_read(&nr_real_swapfiles))
>  		queue_work(shrink_wq, &zswap_shrink_work);
>  check_old:
>  	/*

[Severity: Medium]
Can shrink_worker() spin in an unbounded loop when memory cgroups are
disabled (CONFIG_MEMCG=n or cgroup_disable=memory) and the zswap pool is
filled above the accept threshold by SWP_XSWAP entries?

mm/zswap.c:shrink_worker() {
    ...
	do {
    ...
		if (!memcg && !mem_cgroup_disabled()) {
			if (!attempts && ++failures == MAX_RECLAIM_RETRIES)
				break;

			attempts = 0;
			goto resched;
		}

		ret = shrink_memcg(memcg);
		/* drop the extra reference */
		mem_cgroup_put(memcg);
    ...
		if (ret == -ENOENT)
			goto resched;

		++attempts;

		if (ret && ++failures == MAX_RECLAIM_RETRIES)
			break;

resched:
		cond_resched();
	} while (zswap_total_pages() > thr);
}

When both a real swapfile and an xswap device are active, zswap_store()
queues zswap_shrink_work once the pool is full.  If zswap_list_lru is empty
because the pool is occupied by SWP_XSWAP entries (which are not on the
LRU), shrink_memcg(NULL) returns -ENOENT and jumps to resched without
incrementing failures, while the !memcg termination check is skipped
because mem_cgroup_disabled() is true.  The worker then loops until
external swap frees or faults bring zswap_total_pages() below thr.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109055455.536925-1-baoquan.he@linux.dev?part=1