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

[Christoph Hellwig]

>  };
>  
> +struct swap_ops;
>  /*

Please keep empty spaces before comments.  And keeping forward
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).

> +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.

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.

> -	} 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.