Re: [PATCH v6 05/12] mm, swap: enable THP swapin for vswap entries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mm, swap: out-of-bounds BIO in __swap_cache_add_check() on swapfile THP
- [High] mm, swap: infinite fault loop in do_swap_page() after vswap THP swapoff
--

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

mm, swap: enable THP swapin for vswap entries

This commit enables swapping in large anonymous folios as a unit for vswap
entries when the batch is backed by a contiguous run of physical swap slots on
a synchronous device. Batches backed by zswap or mixed backing continue to be
rejected so the fault path falls back to smaller orders.

> diff --git a/mm/memory.c b/mm/memory.c
> index 9f651d247eef1..430b83900ba87 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4885,9 +4885,10 @@ static unsigned long thp_swapin_suitable_orders(struct vm_fault *vmf)
>  	 * lack handling for such cases, so fallback to swapping in order-0
>  	 * folio.
>  	 *
> -	 * THP swapin for vswap is not supported yet either.
> +	 * Vswap entries are checked later, under the cluster lock in
> +	 * __swap_cache_add_check().
>  	 */
> -	if (is_vswap_entry(entry) || !zswap_never_enabled())
> +	if (!is_vswap_entry(entry) && !zswap_never_enabled())
>  		return 0;
>  
>  	/*

[Severity: High]
Could enabling large folio swapin for vswap entries here lead to a warning and
an infinite page fault retry loop in do_swap_page() if swapoff (or physical
slot reclaim in a later patch in this series) runs before the folio is mapped?

When do_swap_page() -> swapin_sync() -> __swap_cache_alloc_folio() allocates a
large folio in the swap cache for a vswap batch, folio->mapping is NULL, so
!folio_test_anon(folio) remains true until do_swap_page() acquires the pte
lock, validates the pte batch, and calls folio_add_new_anon_rmap().

Inside swapin_sync(), once the synchronous read from the physical swap device
completes, swap_read_end() marks the folio uptodate and calls
folio_unlock(folio) before returning to do_swap_page(), which then re-acquires
the folio lock via folio_lock_or_retry(folio, vmf).

While that large !folio_test_anon(folio) folio is unlocked in the swap cache
(or if do_swap_page() backs out on VM_FAULT_RETRY or !pte_same()), a
concurrent swapoff() on the physical swap device can run the vswap loop in
try_to_unuse() (or vswap_reclaim_phys_slot() in patch 07/12):

mm/swapfile.c:try_to_unuse()
    folio_wait_writeback(folio);
    folio_release_vswap_backing(folio);
    if (unlikely(!folio_test_uptodate(folio)))
        swap_cache_del_folio(folio);
    else
        folio_mark_dirty(folio);

Unlike non-vswap unuse_pte(), which maps the folio into page tables and sets
folio_test_anon(folio), this releases all physical swap slots for the vswap
batch and marks the large folio dirty in the swap cache while
!folio_test_anon(folio) is still true.

Because thp_swapin_suitable_orders() drops the pte lock before swapin_sync(),
any change to a sibling pte in the range before do_swap_page() re-acquires the
pte lock (such as a concurrent madvise, munmap, mprotect, or a forked child
process faulting with only part of the range mapped) causes the full-batch
check in do_swap_page() to fail, leaving nr_pages = 1 != folio_nr_pages(folio):

mm/memory.c:do_swap_page()
    if (!folio_test_anon(folio) && folio_test_large(folio) &&
        nr_pages != folio_nr_pages(folio)) {
        if (!WARN_ON_ONCE(folio_test_dirty(folio)))
            swap_cache_del_folio(folio);
        goto out_nomap;
    }

Because the folio was marked dirty and its physical backing was released,
WARN_ON_ONCE(folio_test_dirty(folio)) fires, swap_cache_del_folio(folio) is
skipped, and do_swap_page() jumps to out_nomap without mapping the pte.

At out_page, folio_free_swap(folio) returns false because the vswap entries
are still referenced by ptes. Neither reclaim (pageout() in mm/vmscan.c
returns PAGE_ACTIVATE when !shmem_mapping(mapping) && !folio_test_anon(folio))
nor folio splitting (folio_check_splittable() in mm/huge_memory.c returns
-EBUSY when !folio->mapping && !is_anon) can evict or split this dirty
!folio_test_anon(folio) large folio, so every subsequent fault retry finds the
same folio via swap_cache_get_folio() and repeats goto out_nomap indefinitely.

> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 3900975c675f7..6c8e8f49e349d 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -174,6 +174,9 @@ static int __swap_cache_add_check(struct swap_cluster_info *ci,
>  	unsigned int ci_off, ci_end;
>  	unsigned long old_tb;
>  	bool is_zero;
> +	struct swap_cluster_info_dynamic *ci_dyn;
> +	enum vswap_backing_type type;
> +	int ret;
>  
>  	lockdep_assert_held(&ci->lock);
>  
> @@ -202,11 +205,17 @@ static int __swap_cache_add_check(struct swap_cluster_info *ci,
>  		return 0;
>  
>  	/*
> -	 * Reject a vswap batch so __swap_cache_alloc_folio falls back to
> -	 * order 0.
> +	 * For a vswap entry batch, reject if the backing is not THP-amenable
> +	 * (e.g. uniformly ZSWAP, or mixed). The order-fallback loop in
> +	 * __swap_cache_alloc_folio will retry with a smaller order on -EBUSY.
>  	 */
> -	if (is_vswap_entry(targ_entry))
> -		return -EBUSY;
> +	if (is_vswap_entry(targ_entry)) {
> +		ci_dyn = container_of(ci, struct swap_cluster_info_dynamic, ci);
> +		ret = __vswap_check_backing(ci_dyn, round_down(ci_off, nr),
> +					    nr, &type);
> +		if (ret != nr || type == VSWAP_ZSWAP)
> +			return -EBUSY;
> +	}

[Severity: Critical]
Does __swap_cache_add_check() or __vswap_check_backing() also need to verify
that the physical backing swap device has SWP_BLKDEV set (or that the physical
slots lie within a single contiguous swap_extent) before allowing a large
folio swapin?

On swapout, cluster_alloc_swap_entry() in mm/swapfile.c rejects high-order
allocations on regular-file swapfiles where SWP_BLKDEV is not set, while
exempting vswap:

mm/swapfile.c:cluster_alloc_swap_entry()
    if (order && !(si->flags & SWP_BLKDEV) && !swap_is_vswap(si))
        return 0;

Regular-file swapfiles map swap page offsets to disk sectors through
si->swap_extent_root, which can be discontiguous at PAGE_SIZE granularity,
whereas swap_bdev_submit_read() and swap_bdev_submit_write() in mm/page_io.c
submit a single bio of size sio->len (>= folio_size(folio)) starting at
swap_entry_sector(sio->entry) without splitting a large folio across
swap_extent boundaries.

When vswap is backed by a fragmented regular-file swapfile on a synchronous
block device (where claim_swapfile(), setup_swap_extents(), and swapon() set
phys_si->bdev, phys_si->ops = &swap_bdev_ops, and SWP_SYNCHRONOUS_IO without
setting SWP_BLKDEV), cluster_alloc_swap_entry() allocates contiguous high-order
virtual swap entries. When those pages are written out to phys_si at order 0
(for example, after splitting when high-order folio_alloc_phys_swap() fails on
!SWP_BLKDEV, during sequential order-0 swapout, or via sequential zswap
writeback in patch 06/12), folio_alloc_phys_swap() can allocate contiguous
physical swap offsets from percpu_swap_cluster.offset[0] while
swap_bdev_can_merge() splits the order-0 write bios at swap_extent boundaries.

On swapin, __vswap_check_backing() in mm/vswap.h only checks that the physical
swap entry values are contiguous:

mm/vswap.h:__vswap_check_backing()
    } else if (first_type == VSWAP_SWAPFILE &&
               vtable_to_phys(vt).val != first_phys.val + i) {
        break;
    }

If [first_phys, first_phys + nr) crosses a swap_extent boundary on a
!SWP_BLKDEV swapfile, swap_bdev_submit_read() submits a single
folio_size(folio) read bio starting at swap_entry_sector(first_phys), reading
past the end of the swap_extent into adjacent filesystem blocks. In addition,
if that large folio stays in the swap cache and is later written out again,
folio_realloc_swap() in mm/swapfile.c returns the existing
vswap_to_phys(vswap_entry) directly without calling folio_alloc_phys_swap(),
causing swap_bdev_submit_write() to issue a single folio_size(folio) write bio
across the swap_extent boundary.

[ ... ]

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