Re: [PATCH 1/2] mm/swap: fix missing locks in swap_reclaim_work()

[Kairui Song]

On Fri, Mar 6, 2026 at 9:58 PM YoungJun Park <youngjun.park@xxxxxxx> wrote:
>
> On Fri, Mar 06, 2026 at 07:50:36PM +0800, Hui Zhu wrote:
> > From: Hui Zhu <zhuhui@xxxxxxxxxx>
>
> Hello Hui Zhu! :)
> >
> > swap_cluster_alloc_table() assumes that the caller holds the following
> > locks:
> > ci->lock
> > percpu_swap_cluster.lock
> > si->global_cluster_lock (required for non-SWP_SOLIDSTATE devices)
> >
> > There are five call paths leading to swap_cluster_alloc_table():
> > swap_alloc_hibernation_slot->cluster_alloc_swap_entry
> > ->alloc_swap_scan_list->isolate_lock_cluster->swap_cluster_alloc_table
> >
> > swap_alloc_slow->cluster_alloc_swap_entry->alloc_swap_scan_list
> > ->isolate_lock_cluster->swap_cluster_alloc_table
> >
> > swap_alloc_hibernation_slot->cluster_alloc_swap_entry
> > ->swap_reclaim_full_clusters->isolate_lock_cluster
> > ->swap_cluster_alloc_table
> >
> > swap_alloc_slow->cluster_alloc_swap_entry->swap_reclaim_full_clusters
> > ->isolate_lock_cluster->swap_cluster_alloc_table
> >
> > swap_reclaim_work->swap_reclaim_full_clusters->isolate_lock_cluster
> > ->swap_cluster_alloc_table
>
> Can isolate_lock_cluster() actually invoke swap_cluster_alloc_table()
> on a full cluster? My understanding is that full clusters already have
> a swap_table allocated, and swap_cluster_alloc_table() is only called
> for free clusters that need a new allocation. If isolate_lock_cluster()
> checks !cluster_table_is_alloced() before calling swap_cluster_alloc_table(),
> wouldn't the full-cluster reclaim path skip that allocation entirely?

Hi all, thanks for the patch and review. That's correct, I don't think
a full cluster will need allocation.

Maybe we can add a VM_WARN to check if `list == si->free_cluster` when
cluster_table_is_alloced is true? Just in case and make things easier
to understand.

BTW there is already a comment in swap_cluster_alloc_table: "Only
cluster isolation from the allocator does table allocation." That
comment can be extended if that's not detailed enough.