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

[Baoquan He]

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?

> 
> But fortunately, init_swap_ops will never fail at this moment so the
> issue never triggers.
> 
> > +
> >         mutex_lock(&swapon_mutex);
> >         prio = DEF_SWAP_PRIO;
> >         if (swap_flags & SWAP_FLAG_PREFER)
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 4b5149173b0e..192401f46de4 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1054,7 +1054,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
> >         folio_set_reclaim(folio);
> >
> >         /* start writeback */
> > -       __swap_writepage(folio, NULL);
> > +       si->ops->write_folio(si, folio, NULL);
> >
> >  out:
> >         if (ret && ret != -EEXIST) {
> > --
> > 2.52.0
> >
> 
> Rest looks good to me!
>