On Wed, Feb 05, 2025 at 11:10:16AM -0800, Darrick J. Wong wrote:
> 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.
>
Note that this would be shortlived content re: my previous comments on
the advance going away, but sure I can change it.
Brian
> --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
> >
> >
>
