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

[Kairui Song]

On Thu, May 14, 2026 at 10:06 AM Baoquan He <baoquan.he@xxxxxxxxx> wrote:
>
> On 05/13/26 at 01:53am, Kairui Song wrote:
> > On Tue, May 12, 2026 at 6:50 PM Baoquan He <baoquan.he@xxxxxxxxx> 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.
> >
> > opeations -> operations
>
> Will fix, thanks.
>
> >
> > >
> > > Suggested-by: Chris Li <chrisl@xxxxxxxxxx>
> > > Acked-by: Chris Li <chrisl@xxxxxxxxxx>
> > > Co-developed-by: Barry Song <baohua@xxxxxxxxxx>
> > > Signed-off-by: Barry Song <baohua@xxxxxxxxxx>
> > > Signed-off-by: Baoquan He <baoquan.he@xxxxxxxxx>
> >
> > A few nitpicks below:
> >
> > > -void __swap_writepage(struct folio *folio, struct swap_iocb **swap_plug)
> > > -{
> > > -       struct swap_info_struct *sis = __swap_entry_to_info(folio->swap);
> > > -
> > > -       VM_BUG_ON_FOLIO(!folio_test_swapcache(folio), folio);
> >
> > This sanify check is dropped and not added back in anywhere. This is
> > fine, but it might be better to have a similar VM_WARN_ON_FOLIO in
> > swap_writeout?
>
> Good catch. In the old code, it's called by both swap_writeout() and
> zswap_writeback_entry(). In zswap_writeback_entry(), the folio is
> allocated in-place, we don't need to worry about it now. I am fine to
> add it back in swap_writeout() because adding it inside all three
> swap_write_xxx() is a little too much.
>
> Cscope tag: __swap_writepage
>    #   line  filename / context / line
>    1    288  /home/bhe/code/linux/mm/page_io.c <<swap_writeout>>
>              __swap_writepage(folio, swap_plug);
>    2   1053  /home/bhe/code/linux/mm/zswap.c <<zswap_writeback_entry>>
>              __swap_writepage(folio, NULL);
>
> >
> > >  void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
> > >  {
> > >         struct swap_info_struct *sis = __swap_entry_to_info(folio->swap);
> > > @@ -642,13 +664,7 @@ void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
> > >         /* We have to read from slower devices. Increase zswap protection. */
> > >         zswap_folio_swapin(folio);
> > >
> > > -       if (data_race(sis->flags & SWP_FS_OPS)) {
> > > -               swap_read_folio_fs(folio, plug);
> > > -       } else if (synchronous) {
> > > -               swap_read_folio_bdev_sync(folio, sis);
> > > -       } else {
> > > -               swap_read_folio_bdev_async(folio, sis);
> > > -       }
> > > +       sis->ops->read_folio(sis, folio, plug);
> > >
> > >  finish:
> > >         if (workingset) {
> > > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > > index 4840fd40f36f..8c42632e6765 100644
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -3780,6 +3780,15 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> > >                 goto free_swap_zswap;
> > >         }
> > >
> > > +       /*
> > > +        * init_swap_ops() sets si->ops based on flags. It does not need
> > > +        * swapon_mutex, and must complete before enable_swap_info()
> > > +        * exposes the device.
> > > +        */
> > > +       error = init_swap_ops(si);
> > > +       if (error)
> > > +               goto bad_swap_unlock_inode;
> >
> > I checked the comment above previously and it looked good. But the
> > error label seems not that correct after double check. inode->i_flags
> > will keep the S_SWAPFILE flag. Maybe something like add a
> > inode->i_flags &= ~S_SWAPFILE here and goto free_swap_zswap. Sorry I
> > didn't check this part carefully last time.
>
> Right. How about moving it zswap_swapon() because it only relies on
> si->flags setting currently?

You mean before zswap_swapon()? Then that sounds good to me.