Re: [PATCH] mm/swap: fix wrong plist empty check in swap_alloc_slow()

[Kairui Song <ryncsn@gmail.com>]

On Fri, Nov 21, 2025 at 2:54 PM YoungJun Park <youngjun.park@lge.com> wrote:
>
> On Thu, Nov 20, 2025 at 10:18:34AM +0800, Kairui Song wrote:
> > On Thu, Nov 20, 2025 at 10:06 AM YoungJun Park <youngjun.park@lge.com> wrote:
> > >
> > > I've investigated this further and noticed something about swap_sync_discard.
> > > During iteration, it doesn't perform an empty check on swap devices. As a
> > > result, if the iteration exits because the next device is full
> > > (deleted by plist at this time), the
> > > operation(swap I/O) fails even when other available swap devices remain.
> > >
> > > Should this be addressed?
> > >
> > > Best Regards,
> > > Youngjun Park
> >
> > Actually after thinking about it again, swap_sync_discard should be
> > looking at swap_active_head, not swap_avail_head. Changing to
> > swap_active_head and also checking SWP_WRITEOK is a right fix I think,
> > and should be good enough.
> >
>
> Hi Kairui,
>
> Let me confirm if I understand your intention correctly.
>
> It seems the following changes would be made:
> - Change from swap_avail_lock to swap_lock
> - After sync_discard, reacquire the lock and check if the list was
>   broken using SWP_WRITEOK instead. If broken, since we can't know
>   the next si, restart the list traversal from the beginning.
>
> The advantage would be that it's only affected by swapoff changes,
> so exceptional condition checks in this logic would occur less
> frequently. Also, using swap_lock would be better in terms of
> contention compared to swap I/O contention.

Yes, swap_lock is better here, I should use that but somehow forgot
about it while working on something else, thanks for helping out!

And maybe you don't need the `SWP_WRITEOK` change, I was thinking that
swapoff sets ~SWP_WRITEOK and then takes the device off
swap_active_head, so to follow that convention you better check the
flag. But actually no, that's not related, the existing
get_swap_device_info is good enough.

Checking `plist_node_empty(&next->list)` just like what you are doing
here is the right way I think.

Thanks!