Re: [PATCH v2 02/18] xfs: convert iomap ops to ->iomap_next()

[Joanne Koong]

On Thu, Jul 2, 2026 at 9:43 AM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>
> On Tue, Jun 30, 2026 at 05:09:17PM -0700, Joanne Koong wrote:
> > Convert xfs iomap_ops to the new ->iomap_next() callback. This uses the
> > iomap_process() helper, which finishes the previous mapping if needed
> > and produces the next one. No functional changes are intended.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> >  fs/xfs/xfs_file.c  |  4 +-
> >  fs/xfs/xfs_iomap.c | 96 +++++++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 88 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 845a97c9b063..7f8bef1a9954 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -857,9 +857,9 @@ xfs_file_dio_write_atomic(
> >                       NULL, 0);
> >
> >       /*
> > -      * The retry mechanism is based on the ->iomap_begin method returning
> > +      * The retry mechanism is based on the ->iomap_next method returning
> >        * -ENOPROTOOPT, which would be when the REQ_ATOMIC-based write is not
> > -      * possible. The REQ_ATOMIC-based method typically not be possible if
> > +      * possible. The REQ_ATOMIC-based method is typically not possible if
> >        * the write spans multiple extents or the disk blocks are misaligned.
> >        */
> >       if (ret == -ENOPROTOOPT && dops == &xfs_direct_write_iomap_ops) {
> > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > index 225c3de88d03..4fa1a5c985db 100644
> > --- a/fs/xfs/xfs_iomap.c
> > +++ b/fs/xfs/xfs_iomap.c
> > @@ -1037,8 +1037,18 @@ xfs_direct_write_iomap_begin(
> >       return error;
> >  }
> >
> > +static int
> > +xfs_direct_write_iomap_next(
> > +     const struct iomap_iter *iter,
> > +     struct iomap            *iomap,
> > +     struct iomap            *srcmap)
>
> <unvarnished brainstream ahead>
>
> Now that I see the callsites, I think the "next" name could use some
> bikeshedding <cough>.  The purpose of this function is either
>
> (a) to look up the first mapping to start iterating;
> (b) to release whatever resources were attached during the current
>     iteration and look up the next mapping to continue iterating; or
> (c) to decide that it's time to stop iterating.
>
> From that it seems obvious to me that xfs_direct_write_iomap_next yields
> iomaps for iteration.  In Python those are called generator functions;
> in Rust they're called objects that implement the Iterator trait (or
> iterators for short).
>
> How about s/iomap_next/iomap_iter/ ?
>
> Then this function would be called xfs_direct_write_iomap_iter, which
> IMO is a closer description of what the function does, which is to say
> iterates iomaps for direct writes.
>
> (Yes, my brain might be polluted with thinking that "iomap next" refers
> to a major shift, in the sense of "linux next". :P)

Thanks for sharing your thoughts. The idea makes sense to me in theory
but I think in rust and python the "iter" naming is reserved for the
iterator object itself and the method / callback for advancing and
yielding the next item in the iteration is named "next" (eg
Iterator::next in rust and __next__() in python)? I wonder if the
s/_next/_iter gets confusing with multiple things in iomap already
called iter (eg the struct iomap_iter, the iomap_iter() function). I'm
not sure, I'm happy to rename this if that's the preference.

Thanks,
Joanne

>
> > +{
> > +     return iomap_process(iter, iomap, srcmap, xfs_direct_write_iomap_begin,
> > +                     NULL);
>
> But then "iomap_iter" sets up a new problem: should iomap_process have a
> new name that goes along with that?  iomap_iter is already taken for the
> legacy path.  If we were emulating python I'd suggest iomap_iter_yield
> but this is C so we get to reinvent everything from scratch so who
> knows.
>
> --D
>