From: Chris Li <chrisl@kernel.org>
To: Baoquan He <baoquan.he@linux.dev>
Cc: linux-mm@kvack.org, akpm@linux-foundation.org,
usama.arif@linux.dev, baohua@kernel.org, kasong@tencent.com,
nphamcs@gmail.com, shikemeng@huaweicloud.com,
youngjun.park@lge.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/3] mm/swap: use swap_ops to register swap device's methods
Date: Fri, 17 Apr 2026 19:30:10 -0700 [thread overview]
Message-ID: <CACePvbWZO-=Nwho9Hfk-2HpuggZyb+Yt306MdA4X_icAt6+Z9w@mail.gmail.com> (raw)
In-Reply-To: <20260417033951.1111038-3-baoquan.he@linux.dev>
On Thu, Apr 16, 2026 at 8:40 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 operations adapted into swap_ops.
>
> Suggested-by: Chris Li <chrisl@kernel.org>
> Co-developed-by: Barry Song <baohua@kernel.org>
> Signed-off-by: Barry Song <baohua@kernel.org>
> Signed-off-by: Baoquan He <baoquan.he@linux.dev>
Overall looks good, I have some nitpick follows.
Acked-by: Chris Li <chrisl@kernel.org>
> ---
> include/linux/swap.h | 2 +
> mm/swap.h | 10 ++++-
> mm/swap_io.c | 102 +++++++++++++++++++++++++------------------
> mm/swapfile.c | 5 +++
> mm/zswap.c | 2 +-
> 5 files changed, 76 insertions(+), 45 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 4b1f13b5bbad..5fdda26f5c1d 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -242,6 +242,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.
> */
> @@ -282,6 +283,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..29bdc679fa98 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);
> +};
> +int init_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 a62c02ab0551..1b054a122f53 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;
> @@ -279,7 +280,7 @@ int swap_writeout(struct folio *folio, struct swap_iocb **swap_plug)
> return AOP_WRITEPAGE_ACTIVATE;
> }
>
> - __swap_writepage(folio, swap_plug);
> + sis->ops->write_folio(sis, folio, swap_plug);
> return 0;
> out_unlock:
> folio_unlock(folio);
> @@ -369,10 +370,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);
>
> @@ -405,8 +407,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;
> @@ -425,8 +428,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;
>
> @@ -442,29 +446,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);
> -}
> -
Nice
> void swap_write_unplug(struct swap_iocb *sio)
> {
> struct iov_iter from;
> @@ -533,9 +514,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);
>
> @@ -567,8 +549,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;
> @@ -589,8 +572,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;
>
> @@ -604,6 +588,44 @@ 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,
> +};
> +
> +int init_swap_ops(struct swap_info_struct *sis)
> +{
> + /*
> + * ->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;
> +
> + if (!sis->ops || !sis->ops->read_folio || !sis->ops->write_folio)
> + return -1;
Nitpick:
For int type error return, you should use -EINVAL or some thing with
error code. If you don't care about error code, change the return type
to bool instead.
> +
> + return 0;
> +}
> +
> void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
> {
> struct swap_info_struct *sis = __swap_entry_to_info(folio->swap);
> @@ -638,13 +660,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);
Nice.
>
> finish:
> if (workingset) {
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 9174f1eeffb0..af81fa212f1e 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3518,6 +3518,11 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> goto bad_swap_unlock_inode;
> }
>
> + if (init_swap_ops(si)) {
> + error = -EINVAL;
> + goto bad_swap_unlock_inode;
> + }
Assume you return the error code above. Here can be
error = init_swap_ops(si)
if (error)
goto ...;
Chris
> +
> si->max = maxpages;
> si->pages = maxpages - 1;
> nr_extents = setup_swap_extents(si, swap_file, &span);
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 0823cadd02b6..b699413a2526 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1061,7 +1061,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
>
>
next prev parent reply other threads:[~2026-04-18 2:30 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-17 3:39 [PATCH v4 0/3] " Baoquan He
2026-04-17 3:39 ` [PATCH v4 1/3] mm/swap: rename mm/page_io.c to mm/swap_io.c Baoquan He
2026-04-17 3:39 ` [PATCH v4 2/3] mm/swap: use swap_ops to register swap device's methods Baoquan He
2026-04-18 2:30 ` Chris Li [this message]
2026-04-17 3:39 ` [PATCH v4 3/3] mm/swap_io.c: rename swap_writepage_* to swap_write_folio_* Baoquan He
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACePvbWZO-=Nwho9Hfk-2HpuggZyb+Yt306MdA4X_icAt6+Z9w@mail.gmail.com' \
--to=chrisl@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=baohua@kernel.org \
--cc=baoquan.he@linux.dev \
--cc=kasong@tencent.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=nphamcs@gmail.com \
--cc=shikemeng@huaweicloud.com \
--cc=usama.arif@linux.dev \
--cc=youngjun.park@lge.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox