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

On Thu, Jul 2, 2026 at 6:21 PM Joanne Koong <[email protected]> wrote:
>
> On Thu, Jul 2, 2026 at 4:59 PM Joanne Koong <[email protected]> wrote:
> >
> > On Thu, Jul 2, 2026 at 9:43 AM Darrick J. Wong <[email protected]> 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 <[email protected]>
> > > > ---
> > > >  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.

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