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

[Baoquan He]

On 04/15/26 at 08:05am, Usama Arif wrote:
> On Wed, 15 Apr 2026 16:56:57 +0800 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.
> 
> s/opeations/operations/

Good catch, thanks.

> 
> > 
> > Suggested-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>
> > ---
> >  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(-)
> > 
...snip...
> > diff --git a/mm/swap_io.c b/mm/swap_io.c
> > index 4bf210bd677e..602e9b871b30 100644
> > --- a/mm/swap_io.c
> > +++ b/mm/swap_io.c
...snip...  
> > @@ -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;
> 
> sis->ops is always non-NULL, and you will return -1 here and
> swapon will always fail with -EINVAL.
> 
> You probably wanted?
> 
>       if (!sis->ops || !sis->ops->read_folio || !sis->ops->write_folio)
>           return -1;

You are right. Think one thing, write another. Will fix it in v4.
Thanks a lot for your careful reviewing.

> 
> > +
> > +	return 0;
> > +}
> > +
> >  void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
> >  {
> >  	struct swap_info_struct *sis = __swap_entry_to_info(folio->swap);