Re: [PATCH v3 16/28] xfs: add bio_set and submit_io for ioend post-processing

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Oct 06, 2023 at 08:49:10PM +0200, Andrey Albershteyn wrote:
> The read IO path provides callout for configuring ioend. This allows
> filesystem to add verification of completed BIOs. One of such tasks
> is verification against fs-verity tree when pages were read. iomap
> allows using custom bio_set with submit_bio() to add ioend
> processing. The xfs_prepare_read_ioend() configures bio->bi_end_io
> which places verification task in the workqueue. The task does
> fs-verity verification and then call back to the iomap to finish IO.
> 
> This patch adds callouts implementation to verify pages with
> fs-verity. Also implements folio operation .verify_folio for direct
> folio verification by fs-verity.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  fs/xfs/xfs_aops.c  | 84 ++++++++++++++++++++++++++++++++++++++++++++--
>  fs/xfs/xfs_aops.h  |  2 ++
>  fs/xfs/xfs_linux.h |  1 +
>  fs/xfs/xfs_super.c |  9 ++++-
>  4 files changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index b413a2dbcc18..fceb0c3de61f 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -26,6 +26,8 @@ struct xfs_writepage_ctx {
>  	unsigned int		cow_seq;
>  };
>  
> +static struct bio_set xfs_read_ioend_bioset;
> +
>  static inline struct xfs_writepage_ctx *
>  XFS_WPC(struct iomap_writepage_ctx *ctx)
>  {
> @@ -548,19 +550,97 @@ xfs_vm_bmap(
>  	return iomap_bmap(mapping, block, &xfs_read_iomap_ops);
>  }
>  
> +static void
> +xfs_read_work_end_io(
> +	struct work_struct *work)
> +{
> +	struct iomap_read_ioend *ioend =
> +		container_of(work, struct iomap_read_ioend, work);
> +	struct bio *bio = &ioend->read_inline_bio;
> +
> +	fsverity_verify_bio(bio);
> +	iomap_read_end_io(bio);
> +	/*
> +	 * The iomap_read_ioend has been freed by bio_put() in
> +	 * iomap_read_end_io()
> +	 */
> +}
> +
> +static void
> +xfs_read_end_io(
> +	struct bio *bio)
> +{
> +	struct iomap_read_ioend *ioend =
> +		container_of(bio, struct iomap_read_ioend, read_inline_bio);
> +	struct xfs_inode	*ip = XFS_I(ioend->io_inode);
> +
> +	WARN_ON_ONCE(!queue_work(ip->i_mount->m_postread_workqueue,
> +					&ioend->work));

If queue_work fails we should EIO the read.

> +}
> +
> +static int
> +xfs_verify_folio(
> +	struct folio	*folio,
> +	loff_t		pos,
> +	unsigned int	len)
> +{
> +	if (fsverity_verify_blocks(folio, len, pos))
> +		return 0;
> +	return -EFSCORRUPTED;
> +}
> +
> +int
> +xfs_init_iomap_bioset(void)

Probably should be marked __init, right?

> +{
> +	return bioset_init(&xfs_read_ioend_bioset,
> +			   4 * (PAGE_SIZE / SECTOR_SIZE),
> +			   offsetof(struct iomap_read_ioend, read_inline_bio),
> +			   BIOSET_NEED_BVECS);

Also, there's nothing specific to XFS in this bioset, is there?
Shouldn't this be in fs/iomap/buffered-io.c and not XFS?

> +}
> +
> +void
> +xfs_free_iomap_bioset(void)
> +{
> +	bioset_exit(&xfs_read_ioend_bioset);
> +}
> +
> +static void
> +xfs_submit_read_bio(
> +	const struct iomap_iter *iter,
> +	struct bio *bio,
> +	loff_t file_offset)
> +{
> +	struct iomap_read_ioend *ioend;
> +
> +	ioend = container_of(bio, struct iomap_read_ioend, read_inline_bio);
> +	ioend->io_inode = iter->inode;
> +	if (fsverity_active(ioend->io_inode)) {
> +		INIT_WORK(&ioend->work, &xfs_read_work_end_io);
> +		ioend->read_inline_bio.bi_end_io = &xfs_read_end_io;
> +	}
> +
> +	submit_bio(bio);
> +}
> +
> +static const struct iomap_readpage_ops xfs_readpage_ops = {
> +	.verify_folio		= &xfs_verify_folio,
> +	.submit_io		= &xfs_submit_read_bio,
> +	.bio_set		= &xfs_read_ioend_bioset,
> +};
> +
>  STATIC int
>  xfs_vm_read_folio(
>  	struct file		*unused,
>  	struct folio		*folio)
>  {
> -	return iomap_read_folio(folio, &xfs_read_iomap_ops, NULL);
> +	return iomap_read_folio(folio, &xfs_read_iomap_ops, &xfs_readpage_ops);

Leave the ops parameter as NULL for non-verity filesystems to avoid the
overhead of indirect calls.  Work data partitions aren't going to enable
verity.

--D

>  }
>  
>  STATIC void
>  xfs_vm_readahead(
>  	struct readahead_control	*rac)
>  {
> -	iomap_readahead(rac, &xfs_read_iomap_ops, NULL);
> +	iomap_readahead(rac, &xfs_read_iomap_ops, &xfs_readpage_ops);
>  }
>  
>  static int
> diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h
> index e0bd68419764..fa7c512b2717 100644
> --- a/fs/xfs/xfs_aops.h
> +++ b/fs/xfs/xfs_aops.h
> @@ -10,5 +10,7 @@ extern const struct address_space_operations xfs_address_space_operations;
>  extern const struct address_space_operations xfs_dax_aops;
>  
>  int	xfs_setfilesize(struct xfs_inode *ip, xfs_off_t offset, size_t size);
> +int	xfs_init_iomap_bioset(void);
> +void	xfs_free_iomap_bioset(void);
>  
>  #endif /* __XFS_AOPS_H__ */
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index e9d317a3dafe..ee213c6dfcaf 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -64,6 +64,7 @@ typedef __u32			xfs_nlink_t;
>  #include <linux/xattr.h>
>  #include <linux/mnt_idmapping.h>
>  #include <linux/debugfs.h>
> +#include <linux/fsverity.h>
>  
>  #include <asm/page.h>
>  #include <asm/div64.h>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 5e1ec5978176..3cdb642961f4 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2375,11 +2375,17 @@ init_xfs_fs(void)
>  	if (error)
>  		goto out_remove_dbg_kobj;
>  
> -	error = register_filesystem(&xfs_fs_type);
> +	error = xfs_init_iomap_bioset();
>  	if (error)
>  		goto out_qm_exit;
> +
> +	error = register_filesystem(&xfs_fs_type);
> +	if (error)
> +		goto out_iomap_bioset;
>  	return 0;
>  
> + out_iomap_bioset:
> +	xfs_free_iomap_bioset();
>   out_qm_exit:
>  	xfs_qm_exit();
>   out_remove_dbg_kobj:
> @@ -2412,6 +2418,7 @@ init_xfs_fs(void)
>  STATIC void __exit
>  exit_xfs_fs(void)
>  {
> +	xfs_free_iomap_bioset();
>  	xfs_qm_exit();
>  	unregister_filesystem(&xfs_fs_type);
>  #ifdef DEBUG
> -- 
> 2.40.1
>