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

[YoungJun Park <youngjun.park@lge.com>]

On Thu, Nov 27, 2025 at 02:23:05AM +0800, Kairui Song wrote:
> On Wed, Nov 26, 2025 at 12:31 AM Youngjun Park <youngjun.park@lge.com> 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;
> 
> Nit: I'd prefer to avoid unnecessary changes like this, `return true`
> is the same thing. No need to send an update for this though, a really
> trivial issue.

Okay, I will revert it to the original code.

> > +
> > +               spin_lock(&swap_lock);
> > +               if (plist_node_empty(&next->list))
> > +                       goto start_over;
> >         }
> > -       spin_unlock(&swap_avail_lock);
> > +       spin_unlock(&swap_lock);
> >
> > -       return false;
> > +       return ret;
> >  }
> >
> >  /**
> > --
> > 2.34.1
> 
> Thanks!
> 
> Acked-by: Kairui Song <kasong@tencent.com>
> 

Thank you
Youngjun Park