Re: [PATCH 04/11] iomap: hide ioends from the generic writeback code

[Joanne Koong <joannelkoong@gmail.com>]

On Tue, Jun 17, 2025 at 3:55 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Replace the ioend pointer in iomap_writeback_ctx with a void *wb_ctx
> one to facilitate non-block, non-ioend writeback for use.  Rename
> the submit_ioend method to writeback_submit and make it mandatory so

I'm confused as to whether this is mandatory or not - afaict from the
code, it's only needed if wpc->wb_ctx is also set. It seems like it's
ok if a filesystem doesn't define ops->writeback_submit so long as
they don't also set wpc->wb_ctx, but if they do set
ops->writeback_submit but don't set wpc->wb_ctx then they shouldn't
expect ->writeback_submit() to be called. It seems like there's a
tight interdependency between the two, it might be worth mentioning
that in the documentation to make that more clear. Or alternatively,
just always calling wpc->ops->writeback_submit() in iomap_writepages()
and having the caller check that wpc->wb_ctx is valid.

> that the generic writeback code stops seeing ioends and bios.
>
> Co-developed-by: Joanne Koong <joannelkoong@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

LGTM

> ---
>  .../filesystems/iomap/operations.rst          | 16 +---
>  block/fops.c                                  |  1 +
>  fs/gfs2/bmap.c                                |  1 +
>  fs/iomap/buffered-io.c                        | 91 ++++++++++---------
>  fs/xfs/xfs_aops.c                             | 60 ++++++------
>  fs/zonefs/file.c                              |  1 +
>  include/linux/iomap.h                         | 19 ++--
>  7 files changed, 93 insertions(+), 96 deletions(-)
>
> diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst
> index b28f215db6e5..ead56b27ec3f 100644
> --- a/Documentation/filesystems/iomap/operations.rst
> +++ b/Documentation/filesystems/iomap/operations.rst
> @@ -285,7 +285,7 @@ The ``ops`` structure must be specified and is as follows:
>   struct iomap_writeback_ops {
>      int (*writeback_range)(struct iomap_writepage_ctx *wpc,
>                 struct folio *folio, u64 pos, unsigned int len, u64 end_pos);
> -    int (*submit_ioend)(struct iomap_writepage_ctx *wpc, int status);
> +    int (*writeback_submit)(struct iomap_writepage_ctx *wpc, int error);
>   };
>
>  The fields are as follows:
> @@ -307,13 +307,7 @@ The fields are as follows:
>      purpose.
>      This function must be supplied by the filesystem.
>
> -  - ``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 before submitting the bio.
> -    This function is optional.
> +  - ``writeback_submit``: Submit the previous built writeback context.

It might be helpful here to add "This function must be supplied by the
filesystem", especially since the paragraph above has that line for
writeback_range()

>
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 063e18476286..047100f94092 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -391,8 +391,7 @@ sector_t iomap_bmap(struct address_space *mapping, sector_t bno,
>  /*
>   * Structure for writeback I/O completions.
>   *
> - * File systems implementing ->submit_ioend (for buffered I/O) or ->submit_io
> - * for direct I/O) can split a bio generated by iomap.  In that case the parent
> + * File systems can split a bio generated by iomap.  In that case the parent
>   * ioend it was split from is recorded in ioend->io_parent.
>   */
>  struct iomap_ioend {
> @@ -416,7 +415,7 @@ static inline struct iomap_ioend *iomap_ioend_from_bio(struct bio *bio)
>
>  struct iomap_writeback_ops {
>         /*
> -        * Required, performs writeback on the passed in range
> +        * Performs writeback on the passed in range

Is the reasoning behind removing "Required" that it's understood that
the default is it's required, so there's no need to explicitly state
that?