[PATCH] iomap: release the folio batch on iomap callback failures

[PATCH] iomap: release the folio batch on iomap callback failures

[Brian Foster]

A sashiko review of an unrelated patch points out that the folio
batch mechanism used for iomap zero range fails to release the batch
in a couple error scenarios. If either calls to ->iomap_end() or
->iomap_begin() fail, the direct return paths bypass the batch
cleanup.

The ->iomap_end() case is not a practical issue at the moment
because there is no user of the mechanism that returns an error from
this path. The ->iomap_begin() case is theoretically possible
because XFS can invoke the fill helper and error out at various
points thereafter. This subtly complicates things because XFS does
not transfer iomap_flags to the iomap data structure in the error
path.

To deal with both of these issues, first make sure to invoke the
cleanup helper in the error path for either fs callback. Second,
update the helper to clear the flag unconditionally and release the
batch so long as it is populated. This more clearly delineates the
purpose of the flag to control the I/O path and not necessarily the
status of the fbatch, so add a comment around this as well.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Assisted-by: LLM
Fixes: 395ed1ef0012 ("iomap: optional zero range dirty folio processing")
Signed-off-by: Brian Foster <bfoster@redhat.com>
---

As noted here[1], I'm aware this conflicts with the outstanding iomap
iter rework. I'm happy to rebase onto that if that is ultimately
preferred. I've got at least one vote to get this in sooner, so this
version is based on 7.2-rc5.

Brian

[1] https://lore.kernel.org/linux-fsdevel/amizdHj6ICgP2xFv@bfoster/

 fs/iomap/iter.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

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

Re: [PATCH] iomap: release the folio batch on iomap callback failures

[Joanne Koong]

On Tue, Jul 28, 2026 at 11:30 AM Brian Foster <bfoster@redhat.com> wrote:
>
> A sashiko review of an unrelated patch points out that the folio
> batch mechanism used for iomap zero range fails to release the batch
> in a couple error scenarios. If either calls to ->iomap_end() or
> ->iomap_begin() fail, the direct return paths bypass the batch
> cleanup.
>
> The ->iomap_end() case is not a practical issue at the moment
> because there is no user of the mechanism that returns an error from
> this path. The ->iomap_begin() case is theoretically possible
> because XFS can invoke the fill helper and error out at various
> points thereafter. This subtly complicates things because XFS does
> not transfer iomap_flags to the iomap data structure in the error
> path.
>
> To deal with both of these issues, first make sure to invoke the
> cleanup helper in the error path for either fs callback. Second,
> update the helper to clear the flag unconditionally and release the
> batch so long as it is populated. This more clearly delineates the
> purpose of the flag to control the I/O path and not necessarily the
> status of the fbatch, so add a comment around this as well.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Assisted-by: LLM
> Fixes: 395ed1ef0012 ("iomap: optional zero range dirty folio processing")
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>
> As noted here[1], I'm aware this conflicts with the outstanding iomap
> iter rework. I'm happy to rebase onto that if that is ultimately
> preferred. I've got at least one vote to get this in sooner, so this
> version is based on 7.2-rc5.
>

Hi Brian,

As I understand your analysis of the bug in [1], given that it's an
unlikely / second-order scenario nobody is hitting in reality, maybe
it'd be easiest if I fold this patch into the iomap iter rework series
as a preparatory patch (keeping your authorship and Fixes: tag), make
the needed changes in the iomap iter series to be compatible with your
patch, and then submit everything together to the vfs-7.3.iomap branch
as v5 of the series? I think that avoids the nontrivial merge conflict
Christian/Stephen would have to deal with.

Alternatively, if you prefer to have this as part of 7.2, I can send a
v5 of the series to try minimizing the conflict, and then send
Christian or Stephen a diff for how to resolve the merge when they hit
it.

Thanks,
Joanne

[1] https://lore.kernel.org/linux-fsdevel/amjztG-DisHYbV9W@bfoster/

Re: [PATCH] iomap: release the folio batch on iomap callback failures

[Brian Foster]

On Tue, Jul 28, 2026 at 12:07:10PM -0700, Joanne Koong wrote:
> On Tue, Jul 28, 2026 at 11:30 AM Brian Foster <bfoster@redhat.com> wrote:
> >
> > A sashiko review of an unrelated patch points out that the folio
> > batch mechanism used for iomap zero range fails to release the batch
> > in a couple error scenarios. If either calls to ->iomap_end() or
> > ->iomap_begin() fail, the direct return paths bypass the batch
> > cleanup.
> >
> > The ->iomap_end() case is not a practical issue at the moment
> > because there is no user of the mechanism that returns an error from
> > this path. The ->iomap_begin() case is theoretically possible
> > because XFS can invoke the fill helper and error out at various
> > points thereafter. This subtly complicates things because XFS does
> > not transfer iomap_flags to the iomap data structure in the error
> > path.
> >
> > To deal with both of these issues, first make sure to invoke the
> > cleanup helper in the error path for either fs callback. Second,
> > update the helper to clear the flag unconditionally and release the
> > batch so long as it is populated. This more clearly delineates the
> > purpose of the flag to control the I/O path and not necessarily the
> > status of the fbatch, so add a comment around this as well.
> >
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Assisted-by: LLM
> > Fixes: 395ed1ef0012 ("iomap: optional zero range dirty folio processing")
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >
> > As noted here[1], I'm aware this conflicts with the outstanding iomap
> > iter rework. I'm happy to rebase onto that if that is ultimately
> > preferred. I've got at least one vote to get this in sooner, so this
> > version is based on 7.2-rc5.
> >
> 
> Hi Brian,
> 
> As I understand your analysis of the bug in [1], given that it's an
> unlikely / second-order scenario nobody is hitting in reality, maybe
> it'd be easiest if I fold this patch into the iomap iter rework series
> as a preparatory patch (keeping your authorship and Fixes: tag), make
> the needed changes in the iomap iter series to be compatible with your
> patch, and then submit everything together to the vfs-7.3.iomap branch
> as v5 of the series? I think that avoids the nontrivial merge conflict
> Christian/Stephen would have to deal with.
> 

That's perfectly fine with me if you're Ok with doing that and nobody
otherwise objects.

Brian

> Alternatively, if you prefer to have this as part of 7.2, I can send a
> v5 of the series to try minimizing the conflict, and then send
> Christian or Stephen a diff for how to resolve the merge when they hit
> it.
> 
> Thanks,
> Joanne
> 
> [1] https://lore.kernel.org/linux-fsdevel/amjztG-DisHYbV9W@bfoster/
> 

Re: [PATCH] iomap: release the folio batch on iomap callback failures

[Joanne Koong]

On Tue, Jul 28, 2026 at 12:12 PM Brian Foster <bfoster@redhat.com> wrote:
>
> On Tue, Jul 28, 2026 at 12:07:10PM -0700, Joanne Koong wrote:
> > On Tue, Jul 28, 2026 at 11:30 AM Brian Foster <bfoster@redhat.com> wrote:
> > >
> > > A sashiko review of an unrelated patch points out that the folio
> > > batch mechanism used for iomap zero range fails to release the batch
> > > in a couple error scenarios. If either calls to ->iomap_end() or
> > > ->iomap_begin() fail, the direct return paths bypass the batch
> > > cleanup.
> > >
> > > The ->iomap_end() case is not a practical issue at the moment
> > > because there is no user of the mechanism that returns an error from
> > > this path. The ->iomap_begin() case is theoretically possible
> > > because XFS can invoke the fill helper and error out at various
> > > points thereafter. This subtly complicates things because XFS does
> > > not transfer iomap_flags to the iomap data structure in the error
> > > path.
> > >
> > > To deal with both of these issues, first make sure to invoke the
> > > cleanup helper in the error path for either fs callback. Second,
> > > update the helper to clear the flag unconditionally and release the
> > > batch so long as it is populated. This more clearly delineates the
> > > purpose of the flag to control the I/O path and not necessarily the
> > > status of the fbatch, so add a comment around this as well.
> > >
> > > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > > Assisted-by: LLM
> > > Fixes: 395ed1ef0012 ("iomap: optional zero range dirty folio processing")
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > >
> > > As noted here[1], I'm aware this conflicts with the outstanding iomap
> > > iter rework. I'm happy to rebase onto that if that is ultimately
> > > preferred. I've got at least one vote to get this in sooner, so this
> > > version is based on 7.2-rc5.
> > >
> >
> > Hi Brian,
> >
> > As I understand your analysis of the bug in [1], given that it's an
> > unlikely / second-order scenario nobody is hitting in reality, maybe
> > it'd be easiest if I fold this patch into the iomap iter rework series
> > as a preparatory patch (keeping your authorship and Fixes: tag), make
> > the needed changes in the iomap iter series to be compatible with your
> > patch, and then submit everything together to the vfs-7.3.iomap branch
> > as v5 of the series? I think that avoids the nontrivial merge conflict
> > Christian/Stephen would have to deal with.
> >
>
> That's perfectly fine with me if you're Ok with doing that and nobody
> otherwise objects.

I'll wait a day for anyone to object, and if no one does, I'll send v5
tomorrow with your patch folded in.

Thanks,
Joanne

>
> Brian
>
> > Alternatively, if you prefer to have this as part of 7.2, I can send a
> > v5 of the series to try minimizing the conflict, and then send
> > Christian or Stephen a diff for how to resolve the merge when they hit
> > it.
> >
> > Thanks,
> > Joanne
> >
> > [1] https://lore.kernel.org/linux-fsdevel/amjztG-DisHYbV9W@bfoster/
> >
>