Re: [PATCH 01/10] iomap: allow the file system to submit the writeback bios

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

On Thu, Dec 19, 2024 at 05:39:06PM +0000, Christoph Hellwig wrote:
> Change ->prepare_ioend to ->submit_ioend and require file systems that
> implement it to submit the bio.  This is needed for file systems that
> do their own work on the bios before submitting them to the block layer
> like btrfs or zoned xfs.  To make this easier also pass the writeback
> context to the method.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good now,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  Documentation/filesystems/iomap/operations.rst | 11 +++++------
>  fs/iomap/buffered-io.c                         | 10 +++++-----
>  fs/xfs/xfs_aops.c                              | 13 +++++++++----
>  include/linux/iomap.h                          | 12 +++++++-----
>  4 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst
> index ef082e5a4e0c..7ef39b13e65c 100644
> --- a/Documentation/filesystems/iomap/operations.rst
> +++ b/Documentation/filesystems/iomap/operations.rst
> @@ -283,7 +283,7 @@ The ``ops`` structure must be specified and is as follows:
>   struct iomap_writeback_ops {
>       int (*map_blocks)(struct iomap_writepage_ctx *wpc, struct inode *inode,
>                         loff_t offset, unsigned len);
> -     int (*prepare_ioend)(struct iomap_ioend *ioend, int status);
> +     int (*submit_ioend)(struct iomap_writepage_ctx *wpc, int status);
>       void (*discard_folio)(struct folio *folio, loff_t pos);
>   };
>  
> @@ -306,13 +306,12 @@ The fields are as follows:
>      purpose.
>      This function must be supplied by the filesystem.
>  
> -  - ``prepare_ioend``: Enables filesystems to transform the writeback
> -    ioend or perform any other preparatory work before the writeback I/O
> -    is submitted.
> +  - ``submit_ioend``: Allows the file systems to hook into writeback bio
> +    submission.
>      This might include pre-write space accounting updates, or installing
>      a custom ``->bi_end_io`` function for internal purposes, such as
>      deferring the ioend completion to a workqueue to run metadata update
> -    transactions from process context.
> +    transactions from process context before submitting the bio.
>      This function is optional.
>  
>    - ``discard_folio``: iomap calls this function after ``->map_blocks``
> @@ -341,7 +340,7 @@ This can happen in interrupt or process context, depending on the
>  storage device.
>  
>  Filesystems that need to update internal bookkeeping (e.g. unwritten
> -extent conversions) should provide a ``->prepare_ioend`` function to
> +extent conversions) should provide a ``->submit_ioend`` function to
>  set ``struct iomap_end::bio::bi_end_io`` to its own function.
>  This function should call ``iomap_finish_ioends`` after finishing its
>  own work (e.g. unwritten extent conversion).
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 955f19e27e47..cdccf11bb3be 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1675,7 +1675,7 @@ static void iomap_writepage_end_bio(struct bio *bio)
>  }
>  
>  /*
> - * Submit the final bio for an ioend.
> + * Submit an ioend.
>   *
>   * If @error is non-zero, it means that we have a situation where some part of
>   * the submission process has failed after we've marked pages for writeback.
> @@ -1694,14 +1694,14 @@ static int iomap_submit_ioend(struct iomap_writepage_ctx *wpc, int error)
>  	 * failure happened so that the file system end I/O handler gets called
>  	 * to clean up.
>  	 */
> -	if (wpc->ops->prepare_ioend)
> -		error = wpc->ops->prepare_ioend(wpc->ioend, error);
> +	if (wpc->ops->submit_ioend)
> +		error = wpc->ops->submit_ioend(wpc, error);
> +	else if (!error)
> +		submit_bio(&wpc->ioend->io_bio);
>  
>  	if (error) {
>  		wpc->ioend->io_bio.bi_status = errno_to_blk_status(error);
>  		bio_endio(&wpc->ioend->io_bio);
> -	} else {
> -		submit_bio(&wpc->ioend->io_bio);
>  	}
>  
>  	wpc->ioend = NULL;
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 559a3a577097..d175853da5ae 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -395,10 +395,11 @@ xfs_map_blocks(
>  }
>  
>  static int
> -xfs_prepare_ioend(
> -	struct iomap_ioend	*ioend,
> +xfs_submit_ioend(
> +	struct iomap_writepage_ctx *wpc,
>  	int			status)
>  {
> +	struct iomap_ioend	*ioend = wpc->ioend;
>  	unsigned int		nofs_flag;
>  
>  	/*
> @@ -420,7 +421,11 @@ xfs_prepare_ioend(
>  	if (xfs_ioend_is_append(ioend) || ioend->io_type == IOMAP_UNWRITTEN ||
>  	    (ioend->io_flags & IOMAP_F_SHARED))
>  		ioend->io_bio.bi_end_io = xfs_end_bio;
> -	return status;
> +
> +	if (status)
> +		return status;
> +	submit_bio(&ioend->io_bio);
> +	return 0;
>  }
>  
>  /*
> @@ -462,7 +467,7 @@ xfs_discard_folio(
>  
>  static const struct iomap_writeback_ops xfs_writeback_ops = {
>  	.map_blocks		= xfs_map_blocks,
> -	.prepare_ioend		= xfs_prepare_ioend,
> +	.submit_ioend		= xfs_submit_ioend,
>  	.discard_folio		= xfs_discard_folio,
>  };
>  
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 5675af6b740c..c0339678d798 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -362,12 +362,14 @@ struct iomap_writeback_ops {
>  			  loff_t offset, unsigned len);
>  
>  	/*
> -	 * Optional, allows the file systems to perform actions just before
> -	 * submitting the bio and/or override the bio end_io handler for complex
> -	 * operations like copy on write extent manipulation or unwritten extent
> -	 * conversions.
> +	 * Optional, allows the file systems to hook into bio submission,
> +	 * including overriding the bi_end_io handler.
> +	 *
> +	 * Returns 0 if the bio was successfully submitted, or a negative
> +	 * error code if status was non-zero or another error happened and
> +	 * the bio could not be submitted.
>  	 */
> -	int (*prepare_ioend)(struct iomap_ioend *ioend, int status);
> +	int (*submit_ioend)(struct iomap_writepage_ctx *wpc, int status);
>  
>  	/*
>  	 * Optional, allows the file system to discard state on a page where
> -- 
> 2.45.2
> 
>