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

On Tue, Jul 28, 2026 at 09:49:40AM -0400, Brian Foster wrote:
> 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?

From my 30000ft view I'd say push the folio leak fix to linus ASAP for
7.2 and work out the merge conflict resolution in for-next and send that
to broonie/linus for 7.3.  But I'm not sure if people are actually
hitting this and not realizing it; or if this is a fix for a theoretical
problem.

--D

> 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;
>  }
> 
>