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

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

On Fri, Feb 07, 2025 at 09:32:50AM -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.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks for the documentation update, even if some of it is temporary.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/iomap/iter.c       | 32 +++++++++++++++++++++++++-------
>  include/linux/iomap.h |  8 ++++++--
>  2 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> index 544cd7a5a16b..0ebcabc7df52 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..d832a540cc72 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -211,8 +211,11 @@ 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.
> - * @processed: The number of bytes processed by the body in the most recent
> - *	iteration, or a negative errno. 0 causes the iteration to stop.
> + * @iter_start_pos: The original start pos for the current iomap. Used for
> + *	incremental iter advance.
> + * @processed: The number of bytes the most recent iteration needs iomap_iter()
> + *	to advance the iter, zero if the iter was already advanced, or a
> + *	negative errno for an error during the operation.
>   * @flags: Zero or more of the iomap_begin flags above.
>   * @iomap: Map describing the I/O iteration
>   * @srcmap: Source map for COW operations
> @@ -221,6 +224,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
> 
>