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

[Brian Foster <bfoster@redhat.com>]

On Wed, Feb 05, 2025 at 11:00:20AM -0800, Darrick J. Wong wrote:
> 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
...
> > @@ -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...
> 

It's not fully tested yet, but FWIW here's what I currently have in
iomap_iter() after the remaining conversions:

	...
        ret = (iter->len > 0) ? 1 : 0;
        if (iter->processed < 0)
                ret = iter->processed;
        else if (!advanced && !stale)
                ret = 0;
        iomap_iter_reset_iomap(iter);
        if (ret <= 0)
                return ret;
	...

Note that iter.processed should only be success (0) or error here and
hence I was planning to subsequently rename it to iter.status.

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

Thanks.

Brian

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