Re: [PATCH v5 2/3] mm/swap: use swap_ops to register swap device's methods

[Baoquan He <baoquan.he@linux.dev>]

On 05/11/26 at 04:37pm, Kairui Song wrote:
> On Mon, May 11, 2026 at 3:43 PM Baoquan He <baoquan.he@linux.dev> wrote:
> >
> > This simplifies codes and makes logic clearer. And also makes later any
> > new swap device type being added easier to handle.
> >
> > Currently there are three types of swap devices: bdev_fs, bdev_sync
> > and bdev_async, and only operations read_folio and write_folio are
> > included. In the future, there could be more swap device types added
> > and more appropriate opeations adapted into swap_ops.
> 
> Hi Baoquan,
> 
> Thanks for the patch. Sorry I was busy with travel and a few other
> series and didn't got a chance to look at a few previous versions.
> 
> > +struct swap_ops {
> > +       void (*read_folio)(struct swap_info_struct *sis,
> > +                       struct folio *folio,
> > +                       struct swap_iocb **plug);
> > +       void (*write_folio)(struct swap_info_struct *sis,
> > +                       struct folio *folio,
> > +                       struct swap_iocb **plug);
> > +};
> 
> Overall, I really like this idea, we can have a cleaner interface
> starting there.
> 
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 9174f1eeffb0..82d2c9b35b11 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -3518,6 +3518,10 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> >                 goto bad_swap_unlock_inode;
> >         }
> >
> > +       error = init_swap_ops(si);
> > +       if (error)
> > +               goto bad_swap_unlock_inode;
> > +
> 
> But this part seems wrong? init_swap_ops is called before
> setup_swap_extents -> swap_activate sets SWP_FS_OPS, or
> SWP_SYNCHRONOUS_IO is set below, so the branches in init_swap_ops
> never take effect, am I right? You need to move this someplace after
> these flags are set. Maybe right before swapon_mutex so the mutex is a
> clean barrier that the swap device will be used and all things before
> that will be seen by users, with some comments.

You are right, this is wrong.

I checked my queued local patches, I added code relatd to
activation/deaction and register/unregister according to Youngjun's
comments. Later Barry helped post v2 when I took leave. I just picked
code following Barry's v2 to post v3. This wrong code defaults to take
bdev_async_swap_ops, so I didn't catch it when testing.

I will post v6 to fix it. Thanks a lot for careful reviewing and
catching it.

Thanks
Baoquan