[09/10] iomap: remove unnecessary advance from iomap_iter()

Commit Message

Brian Foster Feb. 12, 2025, 1:57 p.m. UTC

At this point, all iomap operations have been updated to advance the
iomap_iter directly before returning to iomap_iter(). Therefore, the
complexity of handling both the old and new semantics is no longer
required and can be removed from iomap_iter().

Update iomap_iter() to expect success or failure status in
iter.processed. As a precaution and developer hint to prevent
inadvertent use of old semantics, warn on a positive return code and
fail the operation. Remove the unnecessary advance and simplify the
termination logic.

Signed-off-by: Brian Foster <[e-mail]>
---
 fs/iomap/iter.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

Comments

Christoph Hellwig Feb. 13, 2025, 6:59 a.m. UTC | #1

On Wed, Feb 12, 2025 at 08:57:11AM -0500, Brian Foster wrote:
> +	ret = (iter->len > 0) ? 1 : 0;
> +	if (iter->processed < 0)
> +		ret = iter->processed;
> +	else if (!advanced && !stale)
>  		ret = 0;

Maybe reshuffle this a bit as:

	if (iter->processed < 0)
		ret = iter->processed;
	else if (iter->len == 0 || (!advanced && !stale))
		ret = 0;
	else
		ret = 1;

Otherwise this looks great!

Brian Foster Feb. 13, 2025, 3:27 p.m. UTC | #2

On Wed, Feb 12, 2025 at 10:59:44PM -0800, Christoph Hellwig wrote:
> On Wed, Feb 12, 2025 at 08:57:11AM -0500, Brian Foster wrote:
> > +	ret = (iter->len > 0) ? 1 : 0;
> > +	if (iter->processed < 0)
> > +		ret = iter->processed;
> > +	else if (!advanced && !stale)
> >  		ret = 0;
> 
> Maybe reshuffle this a bit as:
> 
> 	if (iter->processed < 0)
> 		ret = iter->processed;
> 	else if (iter->len == 0 || (!advanced && !stale))
> 		ret = 0;
> 	else
> 		ret = 1;
> 
> Otherwise this looks great!
> 

Ack, I like that better. Thanks for the feedback.

Brian

Patch

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 0ebcabc7df52..1e2e5c834582 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -60,9 +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;
+	ssize_t advanced;
+	u64 olen;
 	int ret;
 
 	trace_iomap_iter(iter, ops, _RET_IP_);
@@ -71,14 +70,11 @@  int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 		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().
+	 * Calculate how far the iter was advanced and the original length bytes
+	 * for ->iomap_end().
 	 */
-	if (!advanced) {
-		advanced = iter->pos - iter->iter_start_pos;
-		olen += advanced;
-	}
+	advanced = iter->pos - iter->iter_start_pos;
+	olen = iter->len + advanced;
 
 	if (ops->iomap_end) {
 		ret = ops->iomap_end(iter->inode, iter->iter_start_pos,
@@ -89,26 +85,20 @@  int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 			return ret;
 	}
 
-	processed = iter->processed;
-	if (processed < 0) {
-		iomap_iter_reset_iomap(iter);
-		return processed;
-	}
+	/* detect old return semantics where this would advance */
+	if (WARN_ON_ONCE(iter->processed > 0))
+		iter->processed = -EIO;
 
 	/*
-	 * 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
+	 * Explicitly terminate on error status or if 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 && !advanced && !stale)
+	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)