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

[Brian Foster <bfoster@redhat.com>]

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@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
> > 
> > 
>