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

[Baoquan He <baoquan.he@linux.dev>]

On 05/12/26 at 10:32pm, Christoph Hellwig wrote:
> >  };
> >  
> > +struct swap_ops;
> >  /*
> 
> Please keep empty spaces before comments.  And keeping forward
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I may not follow, I noticed that all the comments in the code use
tabs for indentation rather than spaces.

> declarations at the beginning of the file makes them much easier to
> organize (although the current swap.h is a complete mess not only in this
> regard).

Sure, will move it at the beginning of linux/swap.h. Thanks.

> 
> > +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 -EINVAL;
> 
> In the long run setting these from the helpers for ->swap_activate would
> make more sense.  Also as ls long as the ops are a small statically
> defined set these checks here are a bit odd.

In fact, except of your ->swap_activate, I have my ->swap_activate too
queued in my local branch. While from your comments, seems we don't have
conflict because we have different concerns.

Please see Youngjun's comment and my reply in this thread:
https://lore.kernel.org/linux-mm/aaWiBaeSCILBCzqd@yjaykim-PowerEdge-T330/

> 
> I guess we just need to land this ASAP, then my swap_activate series and
> then do a big round of cleanups for a coherent swap interface.

This patchset is part I. I posted v1 to collect suggestions, later I
took a leave, then people think the v1 can be merged firstly and helped
to post v2. I later came back and continue to push this part. If you
just found this patch series and come up with those cleanups, could you
wait a moment until we merge this part and part II is posted? I think
the split handling of block and fs in swap you mentioned is super cool,
and is beyond our plan.

> 
> > -	} 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);
> 
> This is a really annoying double-indirection for the SWP_FS_OPS
> case.  I think we should have one level of indirection here.  One
> way to do this would be to remove the swap_rw operation and
> implement the swap_ops directly in nfs and cifs.
> 
>