Re: [RFC PATCH v3 10/18] mm/swap: add priority queue for swap device allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/swap: swap_device_disable() drops swapon_rwsem before masking device
--

commit 01ef0e90906c766114a8ba5bc17c9a8b4eeba142
Author: Kairui Song <kasong@tencent.com>

mm/swap: add priority queue for swap device allocation

This patch introduces a priority queue of same-priority swap device rings
for swap entry allocation, protected by swapon_rwsem and
swap_queue_update_lock, with per-CPU round-robin iterators and tagged
pointers to mask full or disabled devices.

Link: https://lore.kernel.org/20260714-swap-pcp-priq-v1-9-de9b164ed419@tencent.com
Link: https://lore.kernel.org/alZ7UBXweuuOX4qz@yjaykim-PowerEdge-T330
Link: https://lore.kernel.org/77d6da3d-10af-49a1-a356-72aa8b462e85@gmail.com

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 68bed5023299d..2065e566b7a35 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c

[ ... ]

> @@ -1375,41 +1799,44 @@ static bool get_swap_device_info(struct swap_info_struct *si)
>  	return true;
>  }
>  
> -/* Rotate the device and switch to a new cluster */
> -static void swap_alloc_entry(struct folio *folio)
> +static int swap_alloc_entry(struct folio *folio)
>  {
> -	struct swap_info_struct *si, *next;
> +	struct swap_queue_cursor cursor = {};
> +	long nr_pages = folio_nr_pages(folio);
> +	struct swap_info_struct *si;
> +	int nr_iter, ret;
>  
> -	spin_lock(&swap_avail_lock);
> -start_over:
> -	plist_for_each_entry_safe(si, next, &swap_avail_head, avail_list) {
> -		/* Rotate the device and switch to a new cluster */
> -		plist_requeue(&si->avail_list, &swap_avail_head);
> -		spin_unlock(&swap_avail_lock);
> -		if (get_swap_device_info(si)) {
> -			cluster_alloc_swap_entry(si, folio);
> -			put_swap_device(si);
> -			if (folio_test_swapcache(folio))
> -				return;
> -			if (folio_test_large(folio))
> -				return;
> +	percpu_down_read(&swapon_rwsem);
> +	migrate_disable();
> +	for (nr_iter = 0;; nr_iter++) {
> +		si = swap_queue_get_device(nr_pages, nr_iter, &cursor);
> +		if (IS_ERR(si)) {
> +			ret = PTR_ERR(si);
> +			if (ret == -EBUSY)
> +				continue;
> +			break;
> +		}
> +		cluster_alloc_swap_entry(si, folio);
> +
> +		if (folio_test_swapcache(folio)) {
> +			ret = 0;
> +			break;
>  		}
>  
> -		spin_lock(&swap_avail_lock);
>  		/*
> -		 * if we got here, it's likely that si was almost full before,
> -		 * multiple callers probably all tried to get a page from the
> -		 * same si and it filled up before we could get one; or, the si
> -		 * filled up between us dropping swap_avail_lock.
> -		 * Since we dropped the swap_avail_lock, the swap_avail_list
> -		 * may have been modified; so if next is still in the
> -		 * swap_avail_head list then try it, otherwise start over if we
> -		 * have not gotten any slots.
> +		 * For large allocations, try every device at the same priority,
> +		 * but ask the caller to split instead of falling back to a lower
> +		 * priority ring.
>  		 */
> -		if (plist_node_empty(&next->avail_list))
> -			goto start_over;
> +		if (folio_test_large(folio)) {
> +			cursor.ring_only = true;
> +			continue;
> +		}
>  	}
> -	spin_unlock(&swap_avail_lock);
> +
> +	migrate_enable();
> +	percpu_up_read(&swapon_rwsem);
> +	return ret;
>  }

[ ... ]

> @@ -3082,10 +3509,9 @@ static int swap_device_disable(struct address_space *mapping,
>  	}
>  	vm_unacct_memory(si->pages);
>  
> -	spin_lock(&swap_avail_lock);
> +	spin_lock(&swap_queue_update_lock);
>  	si->flags &= ~SWP_WRITEOK;
> -	spin_unlock(&swap_avail_lock);
> -
> +	spin_unlock(&swap_queue_update_lock);
>  	plist_del(&si->list, &swap_active_head);
>  	total_swap_pages -= si->pages;
>  	atomic_long_sub(si->pages, &nr_swap_pages);

[Severity: High]
Should swap_device_disable() call del_from_avail_list(si, true) before
releasing swapon_rwsem?

In swap_device_disable(), SWP_WRITEOK is cleared and percpu_up_write()
releases swapon_rwsem before del_from_avail_list(si, true) masks si in
swap_queue:

mm/swapfile.c:swap_device_disable() {
	...
	spin_lock(&swap_queue_update_lock);
	si->flags &= ~SWP_WRITEOK;
	spin_unlock(&swap_queue_update_lock);
	plist_del(&si->list, &swap_active_head);
	total_swap_pages -= si->pages;
	atomic_long_sub(si->pages, &nr_swap_pages);

	end = ALIGN(si->max, SWAPFILE_CLUSTER);
unlock:
	percpu_up_write(&swapon_rwsem);
	if (err)
		return err;

	del_from_avail_list(si, true);
	...
}

In the window between percpu_up_write(&swapon_rwsem) and
del_from_avail_list(si, true) acquiring swap_queue_update_lock, a
concurrent reader in folio_alloc_swap() -> swap_alloc_entry() can acquire
swapon_rwsem for reading and obtain the unmasked si from
swap_queue_get_device() with SWP_WRITEOK already cleared.

When swap_alloc_entry() then calls cluster_alloc_swap_entry(si, folio) on
a device with !(si->flags & SWP_WRITEOK), __swap_cluster_alloc_entries()
unconditionally returns false, which can cause two problems:

1. For order-0 folios, cluster_alloc_swap_entry() reaches the higher-order
stealing loop:

mm/swapfile.c:cluster_alloc_swap_entry() {
	...
	/* Order 0 stealing from higher order */
	for (int o = 1; o < SWAP_NR_ORDERS; o++) {
		found = alloc_swap_scan_list(si, &si->frag_clusters[o], folio, true);
		if (found)
			goto done;

		found = alloc_swap_scan_list(si, &si->nonfull_clusters[o], folio, true);
		if (found)
			goto done;
	}
}

If si->frag_clusters[o] (or si->nonfull_clusters[o]) has any cluster with
0 < ci->count < SWAPFILE_CLUSTER, isolate_lock_cluster() isolates ci and
sets ci->flags = CLUSTER_FLAG_NONE. alloc_swap_scan_cluster() finds a free
slot, fails __swap_cluster_alloc_entries() because !(si->flags &
SWP_WRITEOK), and calls relocate_cluster(si, ci), which puts ci right back
onto si->frag_clusters[o]. Because scan_all is true,
alloc_swap_scan_list() will loop forever on the same cluster while holding
the cluster lock (si->percpu_cluster->lock or si->global_cluster->lock),
migrate_disable(), and swapon_rwsem.

2. For large folios, swap_alloc_entry() treats the failed
cluster_alloc_swap_entry(si, folio) call on the disabled device as a
fragmentation failure on an active device at that priority and sets
cursor.ring_only = true. If no other peer in the same priority ring
satisfies the allocation, swap_queue_get_device() returns -E2BIG instead
of falling back to lower-priority rings, forcing the caller to split the
large folio even when lower-priority swap devices have free space.

Later in the series, commit "mm/swap: drop swap active plist" works around
this window in swap_queue_first_usable_bdev() by checking SWP_WRITEOK and
SWAP_USAGE_OFFLIST_BIT in addition to swap_device_masked(), but
swap_device_disable() and swap_alloc_entry() remain exposed to this window.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793106426.git.lianux.mm@gmail.com?part=10