Re: [PATCH 2/3] mm/swapfile.c: use swap_info[] to find the swap device

[Baoquan He <bhe@redhat.com>]

On 10/02/25 at 09:50pm, Chris Li wrote:
> On Thu, Oct 2, 2025 at 7:39 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > On 10/02/25 at 08:59am, Chris Li wrote:
> > > On Tue, Sep 30, 2025 at 9:35 PM Baoquan He <bhe@redhat.com> wrote:
> > > >
> > > > Now, swap_active_head is only used to find a present swap device when
> > > > trying to swapoff it. In fact, swap_info[] is a short array which is
> > > > 32 at maximum, and usually the unused one can be reused, so the
> > > > searching for target mostly only iterates the foremost several used
> > > > slots. And swapoff is a rarely used operation, efficiency is not so
> > > > important. Then it's unnecessary to get a plist to make it.
> > > >
> > > > Here go by iterating swap_info[] to find the swap device instead of
> > > > iterating swap_active_head.
> > > >
> > > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > > ---
> > > >  mm/swapfile.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > > > index 5d71c748a2fe..18b52cc20749 100644
> > > > --- a/mm/swapfile.c
> > > > +++ b/mm/swapfile.c
> > > > @@ -2641,6 +2641,7 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> > > >         struct inode *inode;
> > > >         struct filename *pathname;
> > > >         int err, found = 0;
> > > > +       unsigned int type;
> > > >
> > > >         if (!capable(CAP_SYS_ADMIN))
> > > >                 return -EPERM;
> > > > @@ -2658,7 +2659,8 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> > > >
> > > >         mapping = victim->f_mapping;
> > > >         spin_lock(&swap_lock);
> > > > -       plist_for_each_entry(p, &swap_active_head, list) {
> > > > +       for (type = 0; type < nr_swapfiles; type++) {
> > > > +               p = swap_info[type];
> > >
> > > After some careful thinking of the swapoff behavior, I now consider
> > > this patch buggy.
> > >
> > > On behavior change is that, previously the swapoff is dependent on the
> > > swap_active_list and only swapon device is on that list.
> > >
> > > Now you change to enumerate every device at swap off and depend on the
> > > p->flags & SWP_WRITEOK to filter out the device. Which will go through
> > > all 32 swap devices regardless.
> > >
> > > >                 if (p->flags & SWP_WRITEOK) {
> > > >                         if (p->swap_file->f_mapping == mapping) {
> > >
> > > Considering the following race:
> > >
> > > CPU1:
> > > swapoff( "/dev/sda1")
> > > spin_lock(&swap_lock);
> > >
> > > for (type = 0; type < nr_swapfiles; type++) {
> > >      if (p->flags & SWP_WRITEOK) {
> > >          // found the device.
> > > }
> > > ....
> > > spin_lock(&p->lock);
> > > del_from_avail_list(p, true);
> >   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > plist_del(&p->list, &swap_active_head);
> > > atomic_long_sub(p->pages, &nr_swap_pages);
> > > total_swap_pages -= p->pages;
> > > spin_unlock(&p->lock);
> > > spin_unlock(&swap_lock);
> > >
> > > // at this point the p->flags still have SWP_WRITEOK.
> > > // swap_lock and p->lock are both unlocked.
> >
> > No, SWP_WRITEOK is cleared in del_from_avail_list(). At this point,
> > there's no more SWP_WRITEOK in p->flags when both swap_lock and p->lock
> > are lifted.
> 
> Ah, you are right. I am wrong. I missed the "si->flags &=
> ~SWP_WRITEOK;" inside the del_from_avail_list().
> 
> I read at the end of swapoff it set the si->flags = 0; thinking the
> SWP_WRITEOK was clear there. It is cleared earlier.
> 
> That should be fine. Sorry about the misread.

It's OK, thanks for careful reviewing and checking.