Re: [PATCH v1 13/16] iomap: add a private arg for read and readahead

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Sep 05, 2025 at 10:21:19AM +0800, Gao Xiang wrote:
> 
> 
> On 2025/9/5 07:29, Joanne Koong wrote:
> > On Tue, Sep 2, 2025 at 6:55 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> > > 
> 
> ...
> 
> 
> > > > > 
> > > > > >    int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops,
> > > > > > -             const struct iomap_read_ops *read_ops)
> > > > > > +             const struct iomap_read_ops *read_ops, void *private)
> > > > > >    {
> > > > > >         struct iomap_iter iter = {
> > > > > >                 .inode          = folio->mapping->host,
> > > > > >                 .pos            = folio_pos(folio),
> > > > > >                 .len            = folio_size(folio),
> > > > > > +             .private        = private,
> > > > > >         };
> > > > > 
> > > > > Will this whole work be landed for v6.18?
> > > > > 
> > > > > If not, may I ask if this patch can be shifted advance in this
> > > > > patchset for applying separately (I tried but no luck).
> > > > > 
> > > > > Because I also need some similar approach for EROFS iomap page
> > > > > cache sharing feature since EROFS uncompressed I/Os go through
> > > > > iomap and extra information needs a proper way to pass down to
> > > > > iomap_{begin,end} with extra pointer `.private` too.
> > > > 
> > > > Hi Gao,
> > > > 
> > > > I'm not sure whether this will be landed for v6.18 but I'm happy to
> > > > shift this patch to the beginning of the patchset for applying
> > > > separately.
> > > 
> > > Yeah, thanks.  At least this common patch can be potentially applied
> > > easily (e.g. form a common commit id for both features if really
> > > needed) since other iomap/FUSE patches are not dependency of our new
> > > feature and shouldn't be coupled with our development branch later.
> > > 
> > 
> > Hi Gao,
> > 
> > I'll be dropping this patch in v2 since all the iomap read stuff is
> > going to go through a struct ctx arg instead of through iter->private.
> > Sorry this won't help your use case, but looking forward to seeing your patches.
> 
> Hi Joanne,
> 
> Thanks for your reminder.  Okay, I will check your v2 to know how
> you change then.
> 
> Also, one thing I really think it's helpful for our use cases is
> converting .iomap_begin() at least to pass struct iomap_iter *
> directly rather than (inode, pos, len, flags, iomap, srcmap)
> since:
>   - .iomap_{begin,end}() are introduced before iomap_iter()
>     and struct iomap_iter but those callbacks are basically
>     now passed down some fields of `struct iomap_iter` now;
> 
>   - struct iomap_iter->private then can contain a per-request
>     context so that .iomap_begin() can leverage too;
> 
>   - There are already too many arguments for .iomap_begin(),
>     pass down struct iomap_iter directly could avoid adding
>     another `private` argument to .iomap_begin()..
> 
> Therefore, I do wonder if this change (.iomap_begin() passes
> struct iomap_iter *) is a good idea for the iomap folks, in
> addition that filesystems can specify `struct iomap_iter->private`
> as in this patch.  Since this change is necessary to make our
> page cache sharing feature efficient, I will continue working on
> this soon.

From a source code perspective, I like the idea of cleaning up the
function signature to pass fewer things to ->iomap_begin.  I suspect
that we could simplify it to:

	int (*iomap_begin)(const struct iomap_iter *iter,
			   struct iomap *iomap,
			   struct iomap *srcmap);

That way we preserve the notion that the ->iomap_begin functions aren't
allowed to change the iterator contents except for the two iomaps.

That said, the nice thing about passing so many parameters is that it
probably leads to less pointer chasing in the implementation functions.
I wonder if that makes any difference because extent mapping lookups
likely involve a lot more pointer chasing anyway.  Another benefit is
that since the parameters aren't const, each implementation can (re)use
those variables if they need to.

I think you could simplify iomap_end too:

	int (*iomap_end)(const struct iomap_iter *iter,
			 loff_t pos, u64 length,
			 size_t written);

and make ->iomap_end implementations extract iter->flags and iter->iomap
themselves if they want to.  I don't like how xfs_iomap.c abuses
container_of to extract the iter from the iomap pointer.

(But not enough to have written patches fixing any of this. :P)

> Another thing I want to discuss (but it's less important for our
> recent features) is the whole callback hook model of iomap.
> 
> Basically the current model does mean if any filesystem doesn't
> fulfill the iomap standard flow, it has to add some customized
> callback hook somewhere to modify the code flow then (or introduce
> a new special flag and move their specific logic into iomap/
> itself even other fses may not need this), but the hook way will
> cause increased indirect calls for them, currently we have
> `struct iomap_ops`, `struct iomap_writeback_ops` and
> `struct iomap_dio_ops`, if some another filesystem (when converting
> buffer I/Os for example or adding {pre,post}-processing ) have
> specified timing, it needs to add new hooks then.
> 
> I do wonder if it's possible to convert iomap to get rid of the
> indirect-call model by just providing helper kAPIs instead,
> take .read_folio / .fiemap for example e.g.
> 
>    xxxfs_read_folio:
>       loop iomap_iter
>         xxxfs_iomap_begin();
> 	iomap_readpage_bio_advance(); [ or if a fs is non-bio
>              based, spliting more low-level helpers for them. ]
>         xxxfs_iomap_end();
> 
>    xxxfs_fiemap():
>       iomap_fiemap_begin
>       loop iomap_iter
>         xxxfs_iomap_begin();
>         iomap_readpage_fiemap_advance()
>         xxxfs_iomap_end();
>       iomap_fiemap_end
> So that each fs can use those helpers flexibly instead of diging
> into adding various new indirect call hooks or moving customized
> logic into iomap/ itself.

Yes, it's quite possible to push the iomap iteration control down into
the filesystems to avoid the indirect calls.  That might make things
faster, though I have no idea what sort of performance impact that will
have.

> I don't have a specific example  because currently we don't have
> direct issue against standard iomap flow on our uncompressed
> path, but after a quick glance of other potential users who try
> to convert their buffer I/Os to iomap, I had such impression in
> my head for a while.

OTOH making it easier for non-disk filesystems to use iomap but supply
their own IO mechanism (transformed bios, memcpy, etc) makes a far more
compelling argument for doing this painful(?) treewide change IMO.

--D

> Thanks,
> Gao Xiang
> 
> > 
> > 
> > Thanks,
> > Joanne
> > 
> > > Thanks,
> > > Gao Xiang
> > > 
> > > > 
> > > > Thanks,
> > > > Joanne
> > > > > 
> > > > > Thanks,
> > > > > Gao Xiang
> > > 
> 
>