Re: [PATCH v5 06/10] iomap: export iomap_iter_advance() and return remaining length

[Brian Foster <bfoster@redhat.com>]

On Wed, Feb 05, 2025 at 10:58:01AM -0800, Darrick J. Wong wrote:
> On Wed, Feb 05, 2025 at 08:58:17AM -0500, Brian Foster wrote:
> > As a final step for generic iter advance, export the helper and
> > update it to return the remaining length of the current iteration
> > after the advance. This will usually be 0 in the iomap_iter() case,
> > but will be useful for the various operations that iterate on their
> > own and will be updated to advance as they progress.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/iomap/iter.c       | 22 ++++++++--------------
> >  include/linux/iomap.h |  1 +
> >  2 files changed, 9 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> > index 8e0746ad80bd..cdba24dbbfd7 100644
> > --- a/fs/iomap/iter.c
> > +++ b/fs/iomap/iter.c
> > @@ -15,22 +15,16 @@ static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
> >  }
> >  
> >  /*
> > - * Advance to the next range we need to map.
> > - *
> > - * If the iomap is marked IOMAP_F_STALE, it means the existing map was not fully
> > - * processed - it was aborted because the extent the iomap spanned may have been
> > - * changed during the operation. In this case, the iteration behaviour is to
> > - * remap the unprocessed range of the iter, and that means we may need to remap
> > - * even when we've made no progress (i.e. count = 0). Hence the "finished
> > - * iterating" case needs to distinguish between (count = 0) meaning we are done
> > - * and (count = 0 && stale) meaning we need to remap the entire remaining range.
> > + * Advance the current iterator position and return the length remaining for the
> > + * current mapping.
> 
> This last sentence should state that the remaining length is returned
> via @count as an outparam and not through the function's return value.
> 

Ok.

Brian

> Otherwise looks ok to me.
> 
> --D
> 
> >   */
> > -static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count)
> > +int iomap_iter_advance(struct iomap_iter *iter, u64 *count)
> >  {
> > -	if (WARN_ON_ONCE(count > iomap_length(iter)))
> > +	if (WARN_ON_ONCE(*count > iomap_length(iter)))
> >  		return -EIO;
> > -	iter->pos += count;
> > -	iter->len -= count;
> > +	iter->pos += *count;
> > +	iter->len -= *count;
> > +	*count = iomap_length(iter);
> >  	return 0;
> >  }
> >  
> > @@ -93,7 +87,7 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
> >  	 * advanced at all (i.e. no work was done for some reason) unless the
> >  	 * mapping has been marked stale and needs to be reprocessed.
> >  	 */
> > -	ret = iomap_iter_advance(iter, processed);
> > +	ret = iomap_iter_advance(iter, &processed);
> >  	if (!ret && iter->len > 0)
> >  		ret = 1;
> >  	if (ret > 0 && !iter->processed && !stale)
> > diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> > index f5ca71ac2fa2..f304c602e5fe 100644
> > --- a/include/linux/iomap.h
> > +++ b/include/linux/iomap.h
> > @@ -229,6 +229,7 @@ struct iomap_iter {
> >  };
> >  
> >  int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
> > +int iomap_iter_advance(struct iomap_iter *iter, u64 *count);
> >  
> >  /**
> >   * iomap_length_trim - trimmed length of the current iomap iteration
> > -- 
> > 2.48.1
> > 
> > 
>