Re: [PATCH v5 18/19] mm, swap: do not give an xswap slot to a cgroup that cannot zswap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mm, swap: folio_alloc_swap() returns -E2BIG when !may_zswap
  has no real swap
- [Low] mm, swap: racy nr_xswap_files check in folio_alloc_swap()
  post-alloc retry
--

commit bd28f08f1cf4ece0de7cacd5579217aa023bae3f
Author: Baoquan He <hebaoquan@kylinos.cn>

mm, swap: do not give an xswap slot to a cgroup that cannot zswap

This commit skips SWP_XSWAP swap devices during swap slot allocation when
the folio's cgroup cannot store pages in zswap according to
obj_cgroup_may_zswap().

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 2e5383036fdcb..efb48ef716113 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c

[ ... ]

> @@ -2083,10 +2111,23 @@ int folio_alloc_swap(struct folio *folio)
>  {
>  	unsigned int order = folio_order(folio);
>  	unsigned int size = 1 << order;
> +	/* True unless the gate below finds an xswap device to route away from. */
> +	bool may_zswap = true;
> +	bool zswap_checked = false;
>  
>  	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>  	VM_BUG_ON_FOLIO(!folio_test_uptodate(folio), folio);
>  
> +	/*
> +	 * A folio that zswap will not take must not get an xswap slot. With
> +	 * no xswap device to route away from, skip the lookup; a device that
> +	 * appears before we pick one is handled after the allocation.
> +	 */
> +	if (atomic_read(&nr_xswap_files)) {
> +		may_zswap = folio_may_zswap(folio);
> +		zswap_checked = true;
> +	}
> +
>  	if (order) {
>  		/*
>  		 * Reject large allocation when THP_SWAP is disabled. Check below
> @@ -2107,8 +2148,8 @@ int folio_alloc_swap(struct folio *folio)
>  
>  again:
>  	local_lock(&percpu_swap_cluster.lock);
> -	if (!swap_alloc_fast(folio))
> -		swap_alloc_slow(folio);
> +	if (!swap_alloc_fast(folio, may_zswap))
> +		swap_alloc_slow(folio, may_zswap);
>  	local_unlock(&percpu_swap_cluster.lock);

[Severity: Medium]
Does folio_alloc_swap() return -E2BIG instead of -ENOMEM for a large folio
when may_zswap is false and only xswap devices have free space?

When !may_zswap causes swap_alloc_fast() and swap_alloc_slow() to skip all
SWP_XSWAP devices and no real swap device can satisfy the allocation,
execution jumps to failed:

mm/swapfile.c:folio_alloc_swap() {
    ...
failed:
	if (get_nr_swap_pages() <= 0)
		return -ENOSPC;
	if (mem_cgroup_get_folio_swap_margin(folio) <= 0)
		return -ENOMEM;

	return order ? -E2BIG : -ENOMEM;
}

Because nr_swap_pages includes xswap capacity and
mem_cgroup_get_folio_swap_margin() only checks memory.swap.max, both checks
pass and folio_alloc_swap() returns -E2BIG.  shrink_folio_list() and
shmem_writeout() then split the large folio and retry folio_alloc_swap() on
each order-0 page, which still has may_zswap == false and fails with -ENOMEM
after repeating the cgroup stats flush for every subpage.

>  	if (!order && unlikely(!folio_test_swapcache(folio))) {
> @@ -2116,6 +2157,22 @@ int folio_alloc_swap(struct folio *folio)
>  			goto again;
>  	}
>  
> +	/*
> +	 * The xswap gate was skipped because there was no device when we
> +	 * looked. One may have appeared since; check before keeping the
> +	 * xswap slot the allocator may have picked.
> +	 */
> +	if (unlikely(!zswap_checked && atomic_read(&nr_xswap_files) &&
> +		     folio_test_swapcache(folio) &&
> +		     (__swap_entry_to_info(folio->swap)->flags & SWP_XSWAP))) {
> +		zswap_checked = true;
> +		may_zswap = folio_may_zswap(folio);
> +		if (!may_zswap) {
> +			swap_cache_del_folio(folio);
> +			goto again;
> +		}
> +	}

[Severity: Low]
Can the atomic_read(&nr_xswap_files) check here miss an xswap slot that was
just allocated?

CPU0                                  CPU1
folio_alloc_swap()
  atomic_read(&nr_xswap_files) == 0
                                      xswap_create()
                                        ...
                                          add_to_avail_list()
                                            atomic_add(1, &nr_xswap_files)
  swap_alloc_slow()
    /* allocates SWP_XSWAP slot */
                                      __swapoff()
                                        swap_info_remove()
                                          del_from_avail_list()
                                            atomic_sub(1, &nr_xswap_files)
  if (!zswap_checked &&
      atomic_read(&nr_xswap_files) ...)
    /* reads 0, skips check */        <-- keeps xswap slot when !may_zswap

del_from_avail_list() decrements nr_xswap_files before __swapoff() runs
wait_for_allocation() and try_to_unuse() (which can also fail and call
reinsert_swap_info()).  When that happens, folio_alloc_swap() returns 0 with
an xswap slot for a cgroup that cannot zswap, and swap_writeout() later
fails when zswap_store() rejects the folio.

Since folio_test_swapcache(folio) and
(__swap_entry_to_info(folio->swap)->flags & SWP_XSWAP) already tell whether
the allocated slot is on an xswap device, atomic_read(&nr_xswap_files) can
be dropped from this condition.

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