Re: [PATCH v7 3/3] mm/swap: add unplug callback to swap_ops to fix leaky abstraction

[Baoquan He]

On 05/15/26 at 12:39pm, Barry Song wrote:
> On Fri, May 15, 2026 at 9:58 AM Baoquan He <baoquan.he@xxxxxxxxx> wrote:
> >
> > When swap_ops was introduced, the FS-swap batch submission remained
> > as a standalone swap_write_unplug() that directly called
> > mapping->a_ops->swap_rw(). This meant callers still had implicit
> > knowledge of filesystem internals rather than going through the
> > swap_ops abstraction.
> >
> > Fix this by adding an unplug callback to struct swap_ops.
> > Each ops table provides its own implementation:
> >  - bdev_fs_swap_ops uses the existing FS batch-submission logic
> >  - bdev_sync/bdev_async_swap_ops leave it NULL since block-layer
> >    plugging handles their I/O
> >
> > The swap_iocb now carries a pointer to its ops table so that
> > swap_write_unplug() can dispatch through the callback without
> > the caller needing to know the swap device type.
> >
> > Signed-off-by: Baoquan He <baoquan.he@xxxxxxxxx>
> > ---
> >  mm/page_io.c | 11 ++++++++++-
> >  mm/swap.h    |  1 +
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/page_io.c b/mm/page_io.c
> > index 38b94c560c37..2c36d261ad98 100644
> > --- a/mm/page_io.c
> > +++ b/mm/page_io.c
> > @@ -329,6 +329,7 @@ static void bio_associate_blkg_from_page(struct bio *bio, struct folio *folio)
> >
> >  struct swap_iocb {
> >         struct kiocb            iocb;
> > +       const struct swap_ops   *ops;
> >         struct bio_vec          bvec[SWAP_CLUSTER_MAX];
> >         int                     pages;
> >         int                     len;
> > @@ -401,6 +402,7 @@ static void swap_fs_write_folio(struct swap_info_struct *sis,
> >                 init_sync_kiocb(&sio->iocb, swap_file);
> >                 sio->iocb.ki_complete = sio_write_complete;
> >                 sio->iocb.ki_pos = pos;
> > +               sio->ops = sis->ops;
> >                 sio->pages = 0;
> >                 sio->len = 0;
> >         }
> > @@ -454,7 +456,7 @@ static void swap_bdev_async_write_folio(struct swap_info_struct *sis,
> >         submit_bio(bio);
> >  }
> >
> > -void swap_write_unplug(struct swap_iocb *sio)
> > +static void swap_fs_write_folio_unplug(struct swap_iocb *sio)
> >  {
> >         struct iov_iter from;
> >         struct address_space *mapping = sio->iocb.ki_filp->f_mapping;
> > @@ -466,6 +468,12 @@ void swap_write_unplug(struct swap_iocb *sio)
> >                 sio_write_complete(&sio->iocb, ret);
> >  }
> >
> > +void swap_write_unplug(struct swap_iocb *sio)
> > +{
> > +       if (sio->ops && sio->ops->unplug)
> > +               sio->ops->unplug(sio);
> > +}
> > +
> 
> Hi Baoquan,
> 
> we have already "sis->ops" check in init_swap_ops, do we need !sis->ops
> again?

That's a good point, and no, I dont' think we need it. I will fix it in
v8. Thanks.

> 
> +int init_swap_ops(struct swap_info_struct *sis)
> +{
> + ...
> +
> + if (!sis->ops || !sis->ops->read_folio || !sis->ops->write_folio)
> + return -EINVAL;
> +
> + return 0;
> +}