diff mbox series

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

Message ID 20250205135821.178256-6-bfoster@redhat.com (mailing list archive)
State New
Headers show
Series iomap: incremental per-operation iter advance | expand

Commit Message

Brian Foster Feb. 5, 2025, 1:58 p.m. UTC 
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(-)

Comments

Darrick J. Wong Feb. 5, 2025, 7 p.m. UTC | #1 
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
> 
>
Brian Foster Feb. 5, 2025, 8:25 p.m. UTC | #2 
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
> > 
> > 
>
diff mbox series

Patch

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;
 	iomap_iter_reset_iomap(iter);
 	if (ret <= 0)
 		return ret;