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@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
> 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
>
>
