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

[Chris Li <chrisl@kernel.org>]

On Sat, Mar 28, 2026 at 12:58 AM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Baoquan He <bhe@redhat.com>
>
> 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.

Should add that there is no functional change expected from this patch.


> Suggested-by: Chris Li <chrisl@kernel.org>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Co-developed-by: Barry Song <baohua@kernel.org>
> Signed-off-by: Barry Song <baohua@kernel.org>

Have some nitpicks follows.

> ---
>  include/linux/swap.h |  2 +
>  mm/swap.h            | 10 ++++-
>  mm/swap_io.c         | 99 +++++++++++++++++++++++++-------------------
>  mm/swapfile.c        |  1 +
>  mm/zswap.c           |  3 +-
>  5 files changed, 70 insertions(+), 45 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 1930f81e6be4..b52f30dad72b 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -243,6 +243,7 @@ struct swap_sequential_cluster {
>         unsigned int next[SWAP_NR_ORDERS]; /* Likely next allocation offset */
>  };
>
> +struct swap_ops;
>  /*
>   * The in-memory structure used to track swap areas.
>   */
> @@ -283,6 +284,7 @@ struct swap_info_struct {
>         struct work_struct reclaim_work; /* reclaim worker */
>         struct list_head discard_clusters; /* discard clusters list */
>         struct plist_node avail_list;   /* entry in swap_avail_head */
> +       const struct swap_ops *ops;
>  };
>
>  static inline swp_entry_t page_swap_entry(struct page *page)
> diff --git a/mm/swap.h b/mm/swap.h
> index 161185057993..219dbcb3ffb1 100644
> --- a/mm/swap.h
> +++ b/mm/swap.h
> @@ -217,6 +217,15 @@ extern void __swap_cluster_free_entries(struct swap_info_struct *si,
>  /* linux/mm/swap_io.c */
>  int sio_pool_init(void);
>  struct swap_iocb;
> +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);
> +};
> +void setup_swap_ops(struct swap_info_struct *sis);
>  void swap_read_folio(struct folio *folio, struct swap_iocb **plug);
>  void __swap_read_unplug(struct swap_iocb *plug);
>  static inline void swap_read_unplug(struct swap_iocb *plug)
> @@ -226,7 +235,6 @@ static inline void swap_read_unplug(struct swap_iocb *plug)
>  }
>  void swap_write_unplug(struct swap_iocb *sio);
>  int swap_writeout(struct folio *folio, struct swap_iocb **swap_plug);
> -void __swap_writepage(struct folio *folio, struct swap_iocb **swap_plug);
>
>  /* linux/mm/swap_state.c */
>  extern struct address_space swap_space __read_mostly;
> diff --git a/mm/swap_io.c b/mm/swap_io.c
> index 91b33d955e63..ad0895af6ed8 100644
> --- a/mm/swap_io.c
> +++ b/mm/swap_io.c
> @@ -238,6 +238,7 @@ static void swap_zeromap_folio_clear(struct folio *folio)
>  int swap_writeout(struct folio *folio, struct swap_iocb **swap_plug)
>  {
>         int ret = 0;
> +       struct swap_info_struct *sis = __swap_entry_to_info(folio->swap);
>
>         if (folio_free_swap(folio))
>                 goto out_unlock;
> @@ -283,7 +284,8 @@ int swap_writeout(struct folio *folio, struct swap_iocb **swap_plug)
>         }
>         rcu_read_unlock();
>
> -       __swap_writepage(folio, swap_plug);
> +       VM_WARN_ON_ONCE(!sis->ops || !sis->ops->write_folio);
> +       sis->ops->write_folio(sis, folio, swap_plug);
>         return 0;
>  out_unlock:
>         folio_unlock(folio);
> @@ -373,10 +375,11 @@ static void sio_write_complete(struct kiocb *iocb, long ret)
>         mempool_free(sio, sio_pool);
>  }
>
> -static void swap_writepage_fs(struct folio *folio, struct swap_iocb **swap_plug)
> +static void swap_writepage_fs(struct swap_info_struct *sis,
> +                             struct folio *folio,
> +                             struct swap_iocb **swap_plug)
>  {
>         struct swap_iocb *sio = swap_plug ? *swap_plug : NULL;
> -       struct swap_info_struct *sis = __swap_entry_to_info(folio->swap);
>         struct file *swap_file = sis->swap_file;
>         loff_t pos = swap_dev_pos(folio->swap);
>
> @@ -409,8 +412,9 @@ static void swap_writepage_fs(struct folio *folio, struct swap_iocb **swap_plug)
>                 *swap_plug = sio;
>  }
>
> -static void swap_writepage_bdev_sync(struct folio *folio,
> -               struct swap_info_struct *sis)
> +static void swap_writepage_bdev_sync(struct swap_info_struct *sis,
> +                                    struct folio *folio,
> +                                    struct swap_iocb **plug)
>  {
>         struct bio_vec bv;
>         struct bio bio;
> @@ -429,8 +433,9 @@ static void swap_writepage_bdev_sync(struct folio *folio,
>         __end_swap_bio_write(&bio);
>  }
>
> -static void swap_writepage_bdev_async(struct folio *folio,
> -               struct swap_info_struct *sis)
> +static void swap_writepage_bdev_async(struct swap_info_struct *sis,
> +                                     struct folio *folio,
> +                                     struct swap_iocb **plug)
>  {
>         struct bio *bio;
>
> @@ -446,29 +451,6 @@ static void swap_writepage_bdev_async(struct folio *folio,
>         submit_bio(bio);
>  }
>
> -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);
> -       /*
> -        * ->flags can be updated non-atomically,
> -        * but that will never affect SWP_FS_OPS, so the data_race
> -        * is safe.
> -        */
> -       if (data_race(sis->flags & SWP_FS_OPS))
> -               swap_writepage_fs(folio, swap_plug);
> -       /*
> -        * ->flags can be updated non-atomically,
> -        * but that will never affect SWP_SYNCHRONOUS_IO, so the data_race
> -        * is safe.
> -        */
> -       else if (data_race(sis->flags & SWP_SYNCHRONOUS_IO))
> -               swap_writepage_bdev_sync(folio, sis);
> -       else
> -               swap_writepage_bdev_async(folio, sis);
> -}
> -
>  void swap_write_unplug(struct swap_iocb *sio)
>  {
>         struct iov_iter from;
> @@ -537,9 +519,10 @@ static bool swap_read_folio_zeromap(struct folio *folio)
>         return true;
>  }
>
> -static void swap_read_folio_fs(struct folio *folio, struct swap_iocb **plug)
> +static void swap_read_folio_fs(struct swap_info_struct *sis,
> +                              struct folio *folio,
> +                              struct swap_iocb **plug)
>  {
> -       struct swap_info_struct *sis = __swap_entry_to_info(folio->swap);
>         struct swap_iocb *sio = NULL;
>         loff_t pos = swap_dev_pos(folio->swap);
>
> @@ -571,8 +554,9 @@ static void swap_read_folio_fs(struct folio *folio, struct swap_iocb **plug)
>                 *plug = sio;
>  }
>
> -static void swap_read_folio_bdev_sync(struct folio *folio,
> -               struct swap_info_struct *sis)
> +static void swap_read_folio_bdev_sync(struct swap_info_struct *sis,
> +                                     struct folio *folio,
> +                                     struct swap_iocb **plug)
>  {
>         struct bio_vec bv;
>         struct bio bio;
> @@ -593,8 +577,9 @@ static void swap_read_folio_bdev_sync(struct folio *folio,
>         put_task_struct(current);
>  }
>
> -static void swap_read_folio_bdev_async(struct folio *folio,
> -               struct swap_info_struct *sis)
> +static void swap_read_folio_bdev_async(struct swap_info_struct *sis,
> +                                      struct folio *folio,
> +                                      struct swap_iocb **plug)
>  {
>         struct bio *bio;
>
> @@ -608,6 +593,39 @@ static void swap_read_folio_bdev_async(struct folio *folio,
>         submit_bio(bio);
>  }
>
> +static const struct swap_ops bdev_fs_swap_ops = {
> +       .read_folio = swap_read_folio_fs,
> +       .write_folio = swap_writepage_fs,
> +};
> +
> +static const struct swap_ops bdev_sync_swap_ops = {
> +       .read_folio = swap_read_folio_bdev_sync,
> +       .write_folio = swap_writepage_bdev_sync,
> +};
> +
> +static const struct swap_ops bdev_async_swap_ops = {
> +       .read_folio = swap_read_folio_bdev_async,
> +       .write_folio = swap_writepage_bdev_async,
> +};
> +
> +void setup_swap_ops(struct swap_info_struct *sis)

setup_swap_ops()` needs to return an indication of an error.
The error is that sis->ops is NULL or op->read_folio == NULL or
op->write_folio == NULL.
If there is an error, we should fail the swapon.

See my other comments regardin g the error checking at the dereference side.

> +{
> +       /*
> +        * ->flags can be updated non-atomically, but that will
> +        * never affect SWP_FS_OPS, so the data_race is safe.
> +        */
> +       if (data_race(sis->flags & SWP_FS_OPS))
> +               sis->ops = &bdev_fs_swap_ops;
> +       /*
> +        * ->flags can be updated non-atomically, but that will
> +        * never affect SWP_SYNCHRONOUS_IO, so the data_race is safe.
> +        */
> +       else if (data_race(sis->flags & SWP_SYNCHRONOUS_IO))
> +               sis->ops = &bdev_sync_swap_ops;
> +       else
> +               sis->ops = &bdev_async_swap_ops;
> +}
> +
>  void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
>  {
>         struct swap_info_struct *sis = __swap_entry_to_info(folio->swap);
> @@ -642,13 +660,8 @@ 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);
> -       }
> +       VM_WARN_ON_ONCE(!sis->ops || !sis->ops->read_folio);

WARN_ON_ONCE is not enough. If we hit a WARN here, the kernel will
crash on the next dereference of ops->read_folios().
In other words, we will get kernel OOPS immediately after the warning.
This makes no make sense.

We should refuse to swapon at setup_swap_ops() above.  Then no
checking here is needed.

> +       sis->ops->read_folio(sis, folio, plug);
>
>  finish:
>         if (workingset) {
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index ff315b752afd..c9eb95141c8f 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3732,6 +3732,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>         si->list.prio = -si->prio;
>         si->avail_list.prio = -si->prio;
>         si->swap_file = swap_file;
> +       setup_swap_ops(si);
>
>         /* Sets SWP_WRITEOK, resurrect the percpu ref, expose the swap device */
>         enable_swap_info(si);
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 4b5149173b0e..9bacb1733e1c 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1054,7 +1054,8 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>         folio_set_reclaim(folio);
>
>         /* start writeback */
> -       __swap_writepage(folio, NULL);
> +       VM_WARN_ON_ONCE(!sis->ops || !sis->ops->write_folio);

Again, WARN_ON is not enough, the kernel will panic here if a warning
is triggered.
Check at setup_swap_ops() and refuse to swapon if ops->write_folio is NULL.

With the other "sis" fix, the patch looks good otherwise.

Chris


> +       si->ops->write_folio(si, folio, NULL);
>
>  out:
>         if (ret && ret != -EEXIST) {
> --
> 2.39.3 (Apple Git-146)
>