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

[Darrick J. Wong]

On Fri, Jul 17, 2026 at 09:49:30AM -0700, Joanne Koong wrote:
> On Thu, Jul 2, 2026 at 6:21 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
> >
> > On Thu, Jul 2, 2026 at 4:59 PM Joanne Koong <joannelkoong@xxxxxxxxx> 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.
> > >
> >
> > Ahh okay, I think I see the point you were trying to make. The
> > callsite functions are like:
> >
> > ssize_t iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *from,
> >                 iomap_next_fn iomap_next,
> >                 const struct iomap_write_ops *write_ops, void *private);
> >
> > int iomap_fsverity_write(struct file *file, loff_t pos, size_t length,
> >                 const void *buf, iomap_next_fn iomap_next,
> >                 const struct iomap_write_ops *write_ops);
> >
> > void iomap_read_folio(iomap_next_fn iomap_next,
> >                 struct iomap_read_folio_ctx *ctx, void *private);
> >
> > void iomap_readahead(iomap_next_fn iomap_next,
> >                 struct iomap_read_folio_ctx *ctx, void *private);
> >
> > etc, so it's not clear from the "iomap_next_fn iomap_next" naming that
> > it handles per-iteration logic, whereas renaming it to "iomap_iter_fn
> > iomap_iter" makes it more explicit. Am I interpreting this correctly?
> 
> Going back to this... "iomap_iter_fn iomap_iter" doesn't feel quite
> right. I think there's too much naming collision with iomap_iter() and
> struct iomap_iter, and I like how in general ->next() matches the
> orthodox naming scheme for advancing an iterator. I think
> "iomap_iter_next_fn next" would be the best of both worlds - I'll
> change it to this for v3. But if you don't agree with this, please let
> me know.

Fine by me :)

--D

> 
> Thanks,
> Joanne
> 
> >
> > I think this is a good idea. If no one has an objection, I'll make
> > this change for v3.
> >
> > 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
> > > >
>