Re: [PATCH v5 05/10] iomap: lift iter termination logic from iomap_iter_advance()

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

On Wed, Feb 05, 2025 at 08:58:16AM -0500, Brian Foster wrote:
> The iter termination logic in iomap_iter_advance() is only needed by
> iomap_iter() to determine whether to proceed with the next mapping
> for an ongoing operation. The old logic sets ret to 1 and then
> terminates if the operation is complete (iter->len == 0) or the
> previous iteration performed no work and the mapping has not been
> marked stale. The stale check exists to allow operations to
> retry the current mapping if an inconsistency has been detected.
> 
> To further genericize iomap_iter_advance(), lift the termination
> logic into iomap_iter() and update the former to return success (0)
> or an error code. iomap_iter() continues on successful advance and
> non-zero iter->len or otherwise terminates in the no progress (and
> not stale) or error cases.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap/iter.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> index 1db16be7b9f0..8e0746ad80bd 100644
> --- a/fs/iomap/iter.c
> +++ b/fs/iomap/iter.c
> @@ -27,17 +27,11 @@ static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
>   */
>  static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count)
>  {
> -	bool stale = iter->iomap.flags & IOMAP_F_STALE;
> -	int ret = 1;
> -
>  	if (WARN_ON_ONCE(count > iomap_length(iter)))
>  		return -EIO;
>  	iter->pos += count;
>  	iter->len -= count;
> -	if (!iter->len || (!count && !stale))
> -		ret = 0;
> -
> -	return ret;
> +	return 0;
>  }
>  
>  static inline void iomap_iter_done(struct iomap_iter *iter)
> @@ -69,6 +63,7 @@ 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;
>  	s64 processed;
>  	int ret;
>  
> @@ -91,8 +86,18 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>  		return processed;
>  	}
>  
> -	/* advance and clear state from the previous iteration */
> +	/*
> +	 * Advance the iter and clear state from the previous iteration. 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.
> +	 */
>  	ret = iomap_iter_advance(iter, processed);
> +	if (!ret && iter->len > 0)
> +		ret = 1;
> +	if (ret > 0 && !iter->processed && !stale)
> +		ret = 0;

I guess I'll wait to see what the rest of the conversion series looks
like...

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  	iomap_iter_reset_iomap(iter);
>  	if (ret <= 0)
>  		return ret;
> -- 
> 2.48.1
> 
>