Re: [PATCH 1/2] mm/swapfile: fix list iteration in swap_sync_discard

[Baoquan He <bhe@redhat.com>]

On 11/26/25 at 01:30am, Youngjun Park wrote:
> swap_sync_discard() has an issue where if the next device becomes full
> and is removed from the plist during iteration, the operation fails
> even when other swap devices with pending discard entries remain
> available.
> 
> Fix by checking plist_node_empty(&next->list) and restarting iteration
> when the next node is removed during discard operations.
> 
> Additionally, switch from swap_avail_lock/swap_avail_head to swap_lock/
> swap_active_head. This means the iteration is only affected by swapoff
> operations rather than frequent availability changes, reducing
> exceptional condition checks and lock contention.
> 
> Fixes: 686ea517f471 ("mm, swap: do not perform synchronous discard during allocation")
> Suggested-by: Kairui Song <kasong@tencent.com>
> Signed-off-by: Youngjun Park <youngjun.park@lge.com>
> ---
>  mm/swapfile.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index d12332423a06..998271aa09c3 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1387,21 +1387,25 @@ static bool swap_sync_discard(void)
>  	bool ret = false;
>  	struct swap_info_struct *si, *next;
>  
> -	spin_lock(&swap_avail_lock);
> -	plist_for_each_entry_safe(si, next, &swap_avail_head, avail_list) {
> -		spin_unlock(&swap_avail_lock);
> +	spin_lock(&swap_lock);
> +start_over:
> +	plist_for_each_entry_safe(si, next, &swap_active_head, list) {
> +		spin_unlock(&swap_lock);
>  		if (get_swap_device_info(si)) {
>  			if (si->flags & SWP_PAGE_DISCARD)
>  				ret = swap_do_scheduled_discard(si);
>  			put_swap_device(si);
>  		}
>  		if (ret)
> -			return true;
> -		spin_lock(&swap_avail_lock);
> +			return ret;
> +
> +		spin_lock(&swap_lock);
> +		if (plist_node_empty(&next->list))
> +			goto start_over;

If there are many si with the same priority, or there are several si
spread in different memcg when swap.tier is available, are we going to
keep looping here to start over and over again possibly? The old code is
supposed to go through the plist to do one round of discarding? Not sure
if I got the code wrong, or the chance it very tiny.

Thanks
Baoquan

>  	}
> -	spin_unlock(&swap_avail_lock);
> +	spin_unlock(&swap_lock);
>  
> -	return false;
> +	return ret;
>  }
>  
>  /**
> -- 
> 2.34.1
>