Re: [PATCH v5 07/10] iomap: support incremental iomap_iter advances

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

On Wed, Feb 05, 2025 at 08:58:18AM -0500, Brian Foster wrote:
> The current iomap_iter iteration model reads the mapping from the
> filesystem, processes the subrange of the operation associated with
> the current mapping, and returns the number of bytes processed back
> to the iteration code. The latter advances the position and
> remaining length of the iter in preparation for the next iteration.
> 
> At the _iter() handler level, this tends to produce a processing
> loop where the local code pulls the current position and remaining
> length out of the iter, iterates it locally based on file offset,
> and then breaks out when the associated range has been fully
> processed.
> 
> This works well enough for current handlers, but upcoming
> enhancements require a bit more flexibility in certain situations.
> Enhancements for zero range will lead to a situation where the
> processing loop is no longer a pure ascending offset walk, but
> rather dictated by pagecache state and folio lookup. Since folio
> lookup and write preparation occur at different levels, it is more
> difficult to manage position and length outside of the iter.
> 
> To provide more flexibility to certain iomap operations, introduce
> support for incremental iomap_iter advances from within the
> operation itself. This allows more granular advances for operations
> that might not use the typical file offset based walk.
> 
> Note that the semantics for operations that use incremental advances
> is slightly different than traditional operations. Operations that
> advance the iter directly are expected to return success or failure
> (i.e. 0 or negative error code) in iter.processed rather than the
> number of bytes processed.

I think this needs to be documented in the code comments for @processed
in iomap.h:

  * @processed: The iteration loop body should set this to a negative
  *     errno if an error occurs during processing; zero if it advanced
  *     the iter itself with iomap_iter_advance; or the number of bytes
  *     processed if it needs iomap_iter to advance the iter.

--D

> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap/iter.c       | 32 +++++++++++++++++++++++++-------
>  include/linux/iomap.h |  3 +++
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> index cdba24dbbfd7..9273ef36d5ae 100644
> --- a/fs/iomap/iter.c
> +++ b/fs/iomap/iter.c
> @@ -35,6 +35,8 @@ static inline void iomap_iter_done(struct iomap_iter *iter)
>  	WARN_ON_ONCE(iter->iomap.offset + iter->iomap.length <= iter->pos);
>  	WARN_ON_ONCE(iter->iomap.flags & IOMAP_F_STALE);
>  
> +	iter->iter_start_pos = iter->pos;
> +
>  	trace_iomap_iter_dstmap(iter->inode, &iter->iomap);
>  	if (iter->srcmap.type != IOMAP_HOLE)
>  		trace_iomap_iter_srcmap(iter->inode, &iter->srcmap);
> @@ -58,6 +60,8 @@ static inline void iomap_iter_done(struct iomap_iter *iter)
>  int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>  {
>  	bool stale = iter->iomap.flags & IOMAP_F_STALE;
> +	ssize_t advanced = iter->processed > 0 ? iter->processed : 0;
> +	u64 olen = iter->len;
>  	s64 processed;
>  	int ret;
>  
> @@ -66,11 +70,22 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>  	if (!iter->iomap.length)
>  		goto begin;
>  
> +	/*
> +	 * If iter.processed is zero, the op may still have advanced the iter
> +	 * itself. Calculate the advanced and original length bytes based on how
> +	 * far pos has advanced for ->iomap_end().
> +	 */
> +	if (!advanced) {
> +		advanced = iter->pos - iter->iter_start_pos;
> +		olen += advanced;
> +	}
> +
>  	if (ops->iomap_end) {
> -		ret = ops->iomap_end(iter->inode, iter->pos, iomap_length(iter),
> -				iter->processed > 0 ? iter->processed : 0,
> -				iter->flags, &iter->iomap);
> -		if (ret < 0 && !iter->processed)
> +		ret = ops->iomap_end(iter->inode, iter->iter_start_pos,
> +				iomap_length_trim(iter, iter->iter_start_pos,
> +						  olen),
> +				advanced, iter->flags, &iter->iomap);
> +		if (ret < 0 && !advanced)
>  			return ret;
>  	}
>  
> @@ -81,8 +96,11 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>  	}
>  
>  	/*
> -	 * Advance the iter and clear state from the previous iteration. Use
> -	 * iter->len to determine whether to continue onto the next mapping.
> +	 * Advance the iter and clear state from the previous iteration. This
> +	 * passes iter->processed because that reflects the bytes processed but
> +	 * not yet advanced by the iter handler.
> +	 *
> +	 * Use iter->len to determine whether to continue onto the next mapping.
>  	 * Explicitly terminate in the case where the current iter has not
>  	 * advanced at all (i.e. no work was done for some reason) unless the
>  	 * mapping has been marked stale and needs to be reprocessed.
> @@ -90,7 +108,7 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>  	ret = iomap_iter_advance(iter, &processed);
>  	if (!ret && iter->len > 0)
>  		ret = 1;
> -	if (ret > 0 && !iter->processed && !stale)
> +	if (ret > 0 && !advanced && !stale)
>  		ret = 0;
>  	iomap_iter_reset_iomap(iter);
>  	if (ret <= 0)
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index f304c602e5fe..0135a7f8dd83 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -211,6 +211,8 @@ struct iomap_ops {
>   *	calls to iomap_iter().  Treat as read-only in the body.
>   * @len: The remaining length of the file segment we're operating on.
>   *	It is updated at the same time as @pos.
> + * @iter_start_pos: The original start pos for the current iomap. Used for
> + *	incremental iter advance.
>   * @processed: The number of bytes processed by the body in the most recent
>   *	iteration, or a negative errno. 0 causes the iteration to stop.
>   * @flags: Zero or more of the iomap_begin flags above.
> @@ -221,6 +223,7 @@ struct iomap_iter {
>  	struct inode *inode;
>  	loff_t pos;
>  	u64 len;
> +	loff_t iter_start_pos;
>  	s64 processed;
>  	unsigned flags;
>  	struct iomap iomap;
> -- 
> 2.48.1
> 
>