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

[Baoquan He <bhe@redhat.com>]

On 04/08/26 at 02:26am, Chris Li wrote:
> 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?

Both is fine to me. I will change to do the checking in
setup_swap_ops(). By the way, I tend to rename setup_swap_ops() as
init_swap_ops(), is it OK to you?

Thanks
Baoquan