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

[Chris Li <chrisl@kernel.org>]

On Tue, Apr 7, 2026 at 11:11 PM Baoquan He <bhe@redhat.com> wrote:
>
> On 03/30/26 at 08:30pm, Chris Li wrote:
> ......
> > > @@ -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.
>
> Thanks for your comments.
>
> In the current patches, there's no chance any of these happens:
>   sis->ops is NULL or
>   op->read_folio == NULL or
>   op->write_folio == NULL

I am aware of that. Adding the check in setup_swap_ops() responds to
the WARN_ONCE check on op->write_folio() feedback.
In fact, we should never have a case where op->read_folio or
op->write_folio is NULL. However, I can see that other future swap ops
can be NULL. e.g. Notify swap entry free. It is hard to enforce that
some fields can be NULL and others can't. So having code safeguard it
is fine.

> Because it's a if-else logic in setup_swap_ops(), adding an error
> checking looks a little weird. I think it's worth adding the error
> checking in setup_swap_ops() later when we have new swap_ops added
> and there's potential any of above three cases could happen.

Adding the check in setup_swap_ops() is to remove the useless warning
check on the caller side of op->read_folio(). If you think the check
is not needed setup_swap_ops(), then it is not needed in
op->read_folio() either.
The WARN_ONCE() then kernel panic on reading op->read_folio does not
make sense. The check was added due to one of the feedback that what
if this call back is NULL.

I prefer to keep the `setup_swap_ops()` function responsible for the
NULL check, due to the discussion history surrounding it.
I'm also fine with adding the NULL check later. However, the NULL
check needs to be consistent across the patch series. The other NULL
check inside WARN_ONCE before the caller site op->read_folio()  and
op->write_folio() needs to be removed.

Does that make sense?

Chris