Re: [PATCH v4 01/21] iomap: split iomap_iter() logic into iomap_iter_next()

On Mon, Jul 27, 2026 at 02:17:38PM -0700, Joanne Koong wrote:
> In preparation for changing iomap to use an in-iter (->iomap_next())
> model, move the iomap_iter() logic out into the new iomap_iter_next()
> helper function.
> 
> iomap_iter_next() is added as an inlined helper so it can be called
> directly by ->iomap_next() implementations where the begin()/end()
> callbacks can be direct calls.
> 
> The DEFINE_IOMAP_ITER_NEXT() and DEFINE_IOMAP_ITER_NEXT_END() macros are
> also provided to generate the boilerplate ->iomap_next() wrapper
> functions that simply forward to iomap_iter_next() with the appropriate
> begin/end callbacks. DEFINE_IOMAP_ITER_NEXT() is for the common case
> where there is no end() callback. DEFINE_IOMAP_ITER_NEXT_END() is for
> the case where there is an explicit end() callback.
> 
> No functional change intended. The only code-level difference is that on
> the iomap_end() error path (ret < 0 && !advanced), the old code returned
> with iter.status left as the caller's last value whereas the new code
> zeroes it, but this is not observable in practice as there are no in-tree
> callers that read iter.status after the iteration loop.
> 
> Reviewed-by: Darrick J. Wong <[email protected]>
> Reviewed-by: Fengnan Chang <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Joanne Koong <[email protected]>
> ---
>  fs/iomap/iter.c       | 123 +++++++++++++++++++++---------------------
>  include/linux/iomap.h | 102 +++++++++++++++++++++++++++++------
>  2 files changed, 147 insertions(+), 78 deletions(-)
> 
> diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> index e4a29829591a..66ccb87441ab 100644
> --- a/fs/iomap/iter.c
> +++ b/fs/iomap/iter.c
> @@ -6,15 +6,6 @@
>  #include <linux/iomap.h>
>  #include "trace.h"
>  
> -static inline void iomap_iter_clean_fbatch(struct iomap_iter *iter)
> -{
> -	if (iter->iomap.flags & IOMAP_F_FOLIO_BATCH) {
> -		folio_batch_release(iter->fbatch);
> -		folio_batch_reinit(iter->fbatch);
> -		iter->iomap.flags &= ~IOMAP_F_FOLIO_BATCH;
> -	}
> -}
> -

So hch forwarded me a bit of Sashiko review feedback that called out a
potential folio batch leak on error returns from iomap_begin() or
iomap_end(). Note that I think the ->iomap_end() variant is currently
not an issue because nothing returns error there, but it should be fixed
regardless.

As such, I have the patch below as a fix based on current master. The
idea here is to account for failures from either callback and also the
fact that XFS may not necessarily transfer the iomap_flags on failure. I
considered a couple other options here, like changing that behavior or
using an iter flag, but I think this is the cleanest option.

However this obviously conflicts with this rework series. This isn't a
major conflict IMO.. I'd probably just do the same thing and include the
batch cleanup in the error/exit path of iomap_iter() (or maybe start
passing ret into iomap_iter_done()), but I would need to reintroduce the
helper above. Also after this series I think this could mean a duplicate
call in the termination case where iomap_iter_continue() would have
cleaned things up, but that is relatively harmless. Maybe there is
something incrementally cleaner, but I'm still wrapping my head around
the factoring here..

But anyways, the main thing I wanted to ask is how folks want to handle
this particular bug..? This rework is invasive and looks mostly reviewed
so I don't want to unnecessarily hold it up. I can rebase on top of this
and leave the patch below for -stable, or avoid the helper churn and
post the patch below on its own and rework it into this, or maybe just
tweak this to leave the helper around and avoid some churn that way..
thoughts?

Brian

--- 8< ---

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index e4a29829591a..63617ec48250 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -6,12 +6,18 @@
 #include <linux/iomap.h>
 #include "trace.h"
 
+/*
+ * Release the iter folio batch. Note that the iomap flag is meant to control
+ * the I/O path for the mapping and may not be set in error situations.
+ */
 static inline void iomap_iter_clean_fbatch(struct iomap_iter *iter)
 {
-	if (iter->iomap.flags & IOMAP_F_FOLIO_BATCH) {
+	if (!iter->fbatch)
+		return;
+	iter->iomap.flags &= ~IOMAP_F_FOLIO_BATCH;
+	if (folio_batch_count(iter->fbatch)) {
 		folio_batch_release(iter->fbatch);
 		folio_batch_reinit(iter->fbatch);
-		iter->iomap.flags &= ~IOMAP_F_FOLIO_BATCH;
 	}
 }
 
@@ -79,7 +85,7 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 						  olen),
 				advanced, iter->flags, &iter->iomap);
 		if (ret < 0 && !advanced)
-			return ret;
+			goto error;
 	}
 
 	/* detect old return semantics where this would advance */
@@ -110,7 +116,11 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 	ret = ops->iomap_begin(iter->inode, iter->pos, iter->len, iter->flags,
 			       &iter->iomap, &iter->srcmap);
 	if (ret < 0)
-		return ret;
+		goto error;
 	iomap_iter_done(iter);
 	return 1;
+
+error:
+	iomap_iter_clean_fbatch(iter);
+	return ret;
 }