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

[Kairui Song]

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

>
> 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?

>  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.

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!