Re: [PATCH v2 02/18] xfs: convert iomap ops to ->iomap_next()

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

On Tue, Jun 30, 2026 at 05:09:17PM -0700, Joanne Koong wrote:
> Convert xfs iomap_ops to the new ->iomap_next() callback. This uses the
> iomap_process() helper, which finishes the previous mapping if needed
> and produces the next one. No functional changes are intended.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/xfs/xfs_file.c  |  4 +-
>  fs/xfs/xfs_iomap.c | 96 +++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 88 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 845a97c9b063..7f8bef1a9954 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -857,9 +857,9 @@ xfs_file_dio_write_atomic(
>  			NULL, 0);
>  
>  	/*
> -	 * The retry mechanism is based on the ->iomap_begin method returning
> +	 * The retry mechanism is based on the ->iomap_next method returning
>  	 * -ENOPROTOOPT, which would be when the REQ_ATOMIC-based write is not
> -	 * possible. The REQ_ATOMIC-based method typically not be possible if
> +	 * possible. The REQ_ATOMIC-based method is typically not possible if
>  	 * the write spans multiple extents or the disk blocks are misaligned.
>  	 */
>  	if (ret == -ENOPROTOOPT && dops == &xfs_direct_write_iomap_ops) {
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 225c3de88d03..4fa1a5c985db 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1037,8 +1037,18 @@ xfs_direct_write_iomap_begin(
>  	return error;
>  }
>  
> +static int
> +xfs_direct_write_iomap_next(
> +	const struct iomap_iter *iter,
> +	struct iomap		*iomap,
> +	struct iomap		*srcmap)

<unvarnished brainstream ahead>

Now that I see the callsites, I think the "next" name could use some
bikeshedding <cough>.  The purpose of this function is either

(a) to look up the first mapping to start iterating;
(b) to release whatever resources were attached during the current
    iteration and look up the next mapping to continue iterating; or
(c) to decide that it's time to stop iterating.

From that it seems obvious to me that xfs_direct_write_iomap_next yields
iomaps for iteration.  In Python those are called generator functions;
in Rust they're called objects that implement the Iterator trait (or
iterators for short).

How about s/iomap_next/iomap_iter/ ?

Then this function would be called xfs_direct_write_iomap_iter, which
IMO is a closer description of what the function does, which is to say
iterates iomaps for direct writes.

(Yes, my brain might be polluted with thinking that "iomap next" refers
to a major shift, in the sense of "linux next". :P)

> +{
> +	return iomap_process(iter, iomap, srcmap, xfs_direct_write_iomap_begin,
> +			NULL);

But then "iomap_iter" sets up a new problem: should iomap_process have a
new name that goes along with that?  iomap_iter is already taken for the
legacy path.  If we were emulating python I'd suggest iomap_iter_yield
but this is C so we get to reinvent everything from scratch so who
knows.

--D

> +}
> +
>  const struct iomap_ops xfs_direct_write_iomap_ops = {
> -	.iomap_begin		= xfs_direct_write_iomap_begin,
> +	.iomap_next		= xfs_direct_write_iomap_next,

>  };
>  
>  #ifdef CONFIG_XFS_RT
> @@ -1089,8 +1099,18 @@ xfs_zoned_direct_write_iomap_begin(
>  	return 0;
>  }
>  
> +static int
> +xfs_zoned_direct_write_iomap_next(
> +	const struct iomap_iter *iter,
> +	struct iomap		*iomap,
> +	struct iomap		*srcmap)
> +{
> +	return iomap_process(iter, iomap, srcmap,
> +			xfs_zoned_direct_write_iomap_begin, NULL);
> +}
> +
>  const struct iomap_ops xfs_zoned_direct_write_iomap_ops = {
> -	.iomap_begin		= xfs_zoned_direct_write_iomap_begin,
> +	.iomap_next		= xfs_zoned_direct_write_iomap_next,
>  };
>  #endif /* CONFIG_XFS_RT */
>  
> @@ -1274,8 +1294,18 @@ xfs_atomic_write_cow_iomap_begin(
>  	return error;
>  }
>  
> +static int
> +xfs_atomic_write_cow_iomap_next(
> +	const struct iomap_iter *iter,
> +	struct iomap		*iomap,
> +	struct iomap		*srcmap)
> +{
> +	return iomap_process(iter, iomap, srcmap,
> +			xfs_atomic_write_cow_iomap_begin, NULL);
> +}
> +
>  const struct iomap_ops xfs_atomic_write_cow_iomap_ops = {
> -	.iomap_begin		= xfs_atomic_write_cow_iomap_begin,
> +	.iomap_next		= xfs_atomic_write_cow_iomap_next,
>  };
>  
>  static int
> @@ -1298,9 +1328,18 @@ xfs_dax_write_iomap_end(
>  	return xfs_reflink_end_cow(ip, pos, written);
>  }
>  
> +static int
> +xfs_dax_write_iomap_next(
> +	const struct iomap_iter *iter,
> +	struct iomap		*iomap,
> +	struct iomap		*srcmap)
> +{
> +	return iomap_process(iter, iomap, srcmap, xfs_direct_write_iomap_begin,
> +			xfs_dax_write_iomap_end);
> +}
> +
>  const struct iomap_ops xfs_dax_write_iomap_ops = {
> -	.iomap_begin	= xfs_direct_write_iomap_begin,
> -	.iomap_end	= xfs_dax_write_iomap_end,
> +	.iomap_next	= xfs_dax_write_iomap_next,
>  };
>  
>  /*
> @@ -2168,9 +2207,19 @@ xfs_buffered_write_iomap_end(
>  	return 0;
>  }
>  
> +static int
> +xfs_buffered_write_iomap_next(
> +	const struct iomap_iter *iter,
> +	struct iomap		*iomap,
> +	struct iomap		*srcmap)
> +{
> +	return iomap_process(iter, iomap, srcmap,
> +			xfs_buffered_write_iomap_begin,
> +			xfs_buffered_write_iomap_end);
> +}
> +
>  const struct iomap_ops xfs_buffered_write_iomap_ops = {
> -	.iomap_begin		= xfs_buffered_write_iomap_begin,
> -	.iomap_end		= xfs_buffered_write_iomap_end,
> +	.iomap_next		= xfs_buffered_write_iomap_next,
>  };
>  
>  static int
> @@ -2214,8 +2263,17 @@ xfs_read_iomap_begin(
>  				 shared ? IOMAP_F_SHARED : 0, seq);
>  }
>  
> +static int
> +xfs_read_iomap_next(
> +	const struct iomap_iter *iter,
> +	struct iomap		*iomap,
> +	struct iomap		*srcmap)
> +{
> +	return iomap_process(iter, iomap, srcmap, xfs_read_iomap_begin, NULL);
> +}
> +
>  const struct iomap_ops xfs_read_iomap_ops = {
> -	.iomap_begin		= xfs_read_iomap_begin,
> +	.iomap_next		= xfs_read_iomap_next,
>  };
>  
>  static int
> @@ -2302,8 +2360,17 @@ xfs_seek_iomap_begin(
>  	return error;
>  }
>  
> +static int
> +xfs_seek_iomap_next(
> +	const struct iomap_iter *iter,
> +	struct iomap		*iomap,
> +	struct iomap		*srcmap)
> +{
> +	return iomap_process(iter, iomap, srcmap, xfs_seek_iomap_begin, NULL);
> +}
> +
>  const struct iomap_ops xfs_seek_iomap_ops = {
> -	.iomap_begin		= xfs_seek_iomap_begin,
> +	.iomap_next		= xfs_seek_iomap_next,
>  };
>  
>  static int
> @@ -2349,8 +2416,17 @@ xfs_xattr_iomap_begin(
>  	return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, IOMAP_F_XATTR, seq);
>  }
>  
> +static int
> +xfs_xattr_iomap_next(
> +	const struct iomap_iter *iter,
> +	struct iomap		*iomap,
> +	struct iomap		*srcmap)
> +{
> +	return iomap_process(iter, iomap, srcmap, xfs_xattr_iomap_begin, NULL);
> +}
> +
>  const struct iomap_ops xfs_xattr_iomap_ops = {
> -	.iomap_begin		= xfs_xattr_iomap_begin,
> +	.iomap_next		= xfs_xattr_iomap_next,
>  };
>  
>  int
> -- 
> 2.52.0
> 
>