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

[Darrick J. Wong]

On Thu, Jul 02, 2026 at 04:59:39PM -0700, Joanne Koong wrote:
> 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.

That's a good point that the functions themselves are usually some
variant of next.

> 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.

Any thoughts on iomap_yield()?  Though I can live with iomap_next since
that /is/ the least effort action. :)

--D