Re: [PATCH v2 17/18] iomap: pass iomap_next_fn directly instead of struct iomap_ops

On Thu, Jul 02, 2026 at 05:17:02PM -0700, Joanne Koong wrote:
> On Thu, Jul 2, 2026 at 9:58 AM Darrick J. Wong <[email protected]> wrote:
> >
> > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > > index 3f0932e46fd6..0aa8abc438c1 100644
> > > --- a/fs/iomap/buffered-io.c
> > > +++ b/fs/iomap/buffered-io.c
> > > @@ -626,7 +626,7 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
> > >       return 0;
> > >  }
> > >
> > > -void iomap_read_folio(const struct iomap_ops *ops,
> > > +void iomap_read_folio(iomap_next_fn iomap_next,
> >
> > If you took my earlier suggestion to rename the typedef to
> > iomap_iter_fn, then this parameter ought to be named iter_fn.
> 
> Hmm... maybe at that point, it's self-explanatory enough that the arg
> could just be called "iter" instead of "iter_fn"?

Dunno.  Seeing as we already have variables named "iter" that are the
actual iteration state object, I think it's clearer to leave the
iteration function as "iter_fn".

> >
> > >               struct iomap_read_folio_ctx *ctx, void *private)
> > >  {
> > >       struct folio *folio = ctx->cur_folio;
> > > @@ -650,7 +650,7 @@ void iomap_read_folio(const struct iomap_ops *ops,
> > >               fsverity_readahead(ctx->vi, folio->index,
> > >                                  folio_nr_pages(folio));
> > >
> > > -     while ((ret = iomap_iter(&iter, ops)) > 0) {
> > > +     while ((ret = iomap_iter(&iter, iomap_next)) > 0) {
> > >               iter.status = iomap_read_folio_iter(&iter, ctx,
> > >                               &bytes_submitted);
> > >               iomap_read_submit(&iter, ctx);
> > > @@ -688,22 +688,22 @@ static int iomap_readahead_iter(struct iomap_iter *iter,
> > >
> > >  /**
> > >   * iomap_readahead - Attempt to read pages from a file.
> > > - * @ops: The operations vector for the filesystem.
> > > + * @iomap_next: The iomap_next callback for the filesystem.
> >
> > "The iomap iteration function for the filesystem" ?
> >
> > Using the term "iomap_next" in the definition for iomap_next isn't that
> > helpful.
> 
> Agreed, I'll replace this with your suggestion.

<nod>

> >
> > >       return ret;
> > > @@ -824,16 +824,16 @@ xfs_file_dio_write_atomic(
> > >       unsigned int            iolock = XFS_IOLOCK_SHARED;
> > >       ssize_t                 ret, ocount = iov_iter_count(from);
> > >       unsigned int            dio_flags = 0;
> > > -     const struct iomap_ops  *dops;
> > > +     iomap_next_fn           dops;
> > >
> > >       /*
> > >        * HW offload should be faster, so try that first if it is already
> > >        * known that the write length is not too large.
> > >        */
> > >       if (ocount > xfs_inode_buftarg(ip)->bt_awu_max)
> > > -             dops = &xfs_atomic_write_cow_iomap_ops;
> > > +             dops = xfs_atomic_write_cow_iomap_next;
> > >       else
> > > -             dops = &xfs_direct_write_iomap_ops;
> > > +             dops = xfs_direct_write_iomap_next;
> >
> > Probably ought to be called iter_fn, or at least something that isn't
> > "dops".
> 
> Nice spotting, I'll rename this in the next version.

<nod>

--D

> Thanks,
> Joanne
>