Re: [RFC PATCH v3 12/18] mm/swap: bound synchronous discard during allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/swap: synchronous discard under swapon_rwsem in cluster_alloc_swap_entry()
- [Medium] mm: premature swap allocation failure in cluster_alloc_swap_entry()
--

commit 148b9ddc65b48866a2eb76b6131e127b4a148727
Author: Kairui Song <kasong@tencent.com>

mm/swap: bound synchronous discard during allocation

This commit replaces the global synchronous discard fallback with a
per-device single-cluster discard in cluster_alloc_swap_entry() when a
swap device's free cluster list is empty.

Link: https://lore.kernel.org/all/CAMgjq7CsYhEjvtN85XGkrONYAJxve7gG593TFeOGV-oax++kWA@mail.gmail.com/

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 0a16343a5b03c..5c26682c0bab2 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c

[ ... ]

> @@ -1576,6 +1585,15 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si,
>  		found = alloc_swap_scan_list(si, &si->free_clusters, folio, false);
>  		if (found)
>  			goto done;
> +
> +		if (!discarded) {
> +			swap_alloc_unlock_device(si);
> +			if (swap_discard_one_cluster(si)) {
> +				discarded = true;
> +				goto restart;
> +			}
> +			swap_alloc_lock_device(si);
> +		}
>  	}

[Severity: High]
Is it safe to call swap_discard_one_cluster() here while holding
swapon_rwsem for reading?

Previously, swap_sync_discard() dropped swapon_rwsem after pinning the
swap device before issuing synchronous discard I/O:

mm/swapfile.c:swap_sync_discard() {
	...
	plist_for_each_entry_safe(si, next, &swap_active_head, list) {
		percpu_up_read(&swapon_rwsem);
		if (get_swap_device_info(si)) {
			if (si->flags & SWP_PAGE_DISCARD)
				ret = swap_do_scheduled_discard(si);
			put_swap_device(si);
		}
	...
}

With this change, swap_alloc_entry() holds percpu_down_read(&swapon_rwsem)
and migrate_disable() across cluster_alloc_swap_entry(), which only drops
the device cluster lock via swap_alloc_unlock_device(si) before calling
swap_discard_one_cluster() -> discard_swap_cluster() ->
blkdev_issue_discard(..., GFP_NOIO) -> submit_bio_wait().

If a writer such as swapon(), swapoff(), or pin_hibernation_swap_type()
calls percpu_down_write(&swapon_rwsem) while submit_bio_wait() is waiting
for the REQ_OP_DISCARD bio to complete, new readers calling
percpu_down_read(&swapon_rwsem) in swap_alloc_entry() will block behind
the writer. Could this stall system-wide swap allocation, or deadlock if
a block backend worker (such as ublk or nbd) needs memory reclaim and swap
allocation to complete the in-flight discard request?

Also, on loop-backed swap devices, processing REQ_OP_DISCARD in the loop
worker calls lo_fallocate() -> file->f_op->fallocate(), which acquires
inode_lock() on the backing file. Since swapon() holds inode_lock() while
calling swap_queue_lock_for_add() -> percpu_down_write(&swapon_rwsem),
doesn't waiting for discard completion under percpu_down_read(&swapon_rwsem)
invert the lock ordering between inode_lock() and swapon_rwsem?

[ ... ]

> @@ -2149,14 +2134,8 @@ int folio_alloc_swap(struct folio *folio)
>  		}
>  	}
>  
> -again:
>  	swap_alloc_entry(folio);
>  
> -	if (!order && unlikely(!folio_test_swapcache(folio))) {
> -		if (swap_sync_discard())
> -			goto again;
> -	}

[Severity: Medium]
Can removing the post-reclaim discard retry cause folio_alloc_swap() to
fail prematurely when si->discard_clusters is populated during cluster
reclaim?

In mm/swapfile.c, when cluster_alloc_swap_entry() is entered on a
SWP_PAGE_DISCARD device with both si->free_clusters and
si->discard_clusters initially empty, alloc_swap_scan_list() on
si->free_clusters and swap_discard_one_cluster() both return false,
leaving discarded set to false.

cluster_alloc_swap_entry() then scans si->nonfull_clusters, calls
swap_reclaim_full_clusters(), and scans si->frag_clusters. During these
steps, cluster_reclaim_range() and swap_reclaim_full_clusters() call
__try_to_reclaim_swap() -> swap_cache_del_folio() ->
__swap_cluster_free_entries(). When reclaiming unmapped swapcache folios
drops a cluster's count to zero, free_cluster() schedules it for discard:

mm/swapfile.c:free_cluster() {
	if ((si->flags & (SWP_WRITEOK | SWP_PAGE_DISCARD)) ==
	    (SWP_WRITEOK | SWP_PAGE_DISCARD)) {
		swap_cluster_schedule_discard(si, ci);
		return;
	}
	...
}

Because swap_cluster_schedule_discard() sets ci->flags to
CLUSTER_FLAG_DISCARD and moves the cluster to si->discard_clusters,
cluster_reclaim_range() sees !cluster_is_usable(ci, order) and cannot
allocate from the cluster it just emptied:

mm/swapfile.c:cluster_reclaim_range() {
	...
	spin_lock(&ci->lock);
	/* Recheck after lock drop, another user might have used it. */
	if (!cluster_is_usable(ci, order)) {
		*usable = false;
		return false;
	}
	...
}

Previously, folio_alloc_swap() called swap_sync_discard() after
swap_alloc_entry() failed and retried the allocation, which discarded the
clusters newly queued to si->discard_clusters during reclaim. Now,
cluster_alloc_swap_entry() never checks si->discard_clusters again after
the nonfull, full, and fragment cluster scans, returning
SWAP_ENTRY_INVALID even though discarded is still false and reclaim just
queued discardable clusters.

In addition, when swap_discard_one_cluster() does discard a cluster, it
places the cluster onto the shared si->free_clusters list via
__free_cluster() and releases ci->lock while the caller does not hold
swap_alloc_lock_device(si). If another CPU isolates that cluster from
si->free_clusters before the discarding task re-acquires
swap_alloc_lock_device(si) at the restart label, won't the discarding task
see an empty si->free_clusters list and fail the allocation even when more
clusters remain on si->discard_clusters?

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