On Tue, Sep 9, 2025 at 7:21 PM Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx> wrote:
>
> Hi Joanne,
>
> On 2025/9/9 23:24, Joanne Koong wrote:
> > On Mon, Sep 8, 2025 at 8:14 PM Gao Xiang <xiang@xxxxxxxxxx> wrote:
> >>
> >> Hi Joanne,
> >>
> >> On Mon, Sep 08, 2025 at 11:51:17AM -0700, Joanne Koong wrote:
> >>> Add caller-provided callbacks for read and readahead so that it can be
> >>> used generically, especially by filesystems that are not block-based.
> >>>
> >>> In particular, this:
> >>> * Modifies the read and readahead interface to take in a
> >>> struct iomap_read_folio_ctx that is publicly defined as:
> >>>
> >>> struct iomap_read_folio_ctx {
> >>> const struct iomap_read_ops *ops;
> >>> struct folio *cur_folio;
> >>> struct readahead_control *rac;
> >>> void *private;
> >>> };
> >>>
> >>> where struct iomap_read_ops is defined as:
> >>>
> >>> struct iomap_read_ops {
> >>> int (*read_folio_range)(const struct iomap_iter *iter,
> >>> struct iomap_read_folio_ctx *ctx,
> >>> loff_t pos, size_t len);
> >>> int (*read_submit)(struct iomap_read_folio_ctx *ctx);
> >>> };
> >>>
> >>
> >> No, I don't think `struct iomap_read_folio_ctx` has another
> >> `.private` makes any sense, because:
> >>
> >> - `struct iomap_iter *iter` already has `.private` and I think
> >> it's mainly used for per-request usage; and your new
> >> `.read_folio_range` already passes
> >> `const struct iomap_iter *iter` which has `.private`
> >> I don't think some read-specific `.private` is useful in any
> >> case, also below.
> >>
> >> - `struct iomap_read_folio_ctx` cannot be accessed by previous
> >> .iomap_{begin,end} helpers, which means `struct iomap_read_ops`
> >> is only useful for FUSE read iter/submit logic.
> >>
> >> Also after my change, the prototype will be:
> >>
> >> int iomap_read_folio(const struct iomap_ops *ops,
> >> struct iomap_read_folio_ctx *ctx, void *private2);
> >> void iomap_readahead(const struct iomap_ops *ops,
> >> struct iomap_read_folio_ctx *ctx, void *private2);
> >>
> >> Is it pretty weird due to `.iomap_{begin,end}` in principle can
> >> only use `struct iomap_iter *` but have no way to access
> >> ` struct iomap_read_folio_ctx` to get more enough content for
> >> read requests.
> >
> > Hi Gao,
> >
> > imo I don't think it makes sense to, if I'm understanding what you're
> > proposing correctly, have one shared data pointer between iomap
> > read/readahead and the iomap_{begin,end} helpers because
>
> My main concern is two `private` naming here: I would like to add
> `private` to iomap_read/readahead() much like __iomap_dio_rw() at
> least to make our new feature work efficiently.
>
> >
> > a) I don't think it's guaranteed that the data needed by
> > read/readahead and iomap_{begin,end} is the same. I guess we could
> > combine the data each needs altogether into one struct, but it seems
> > simpler and cleaner to me to just have the two be separate.
> >
> > b) I'm not sure about the erofs use case, but at least for what I'm
> > seeing for fuse and the block-based filesystems currently using iomap,
> > the data needed by iomap read/readahead (eg bios, the fuse
> > fuse_fill_read_data) is irrelevant for iomap_{begin/end} and it seems
> > unclean to expose that extraneous info. (btw I don't think it's true
> > that iomap_iter is mainly used for per-request usage - for readahead
> > for example, iomap_{begin,end} is called before and after we service
> > the entire readahead, not called per request, whereas
> > .read_folio_range() is called per request).
>
> I said `per-request` meant a single sync read or readahead request,
> which is triggered by vfs or mm for example.
>
> >
> > c) imo iomap_{begin,end} is meant to be a more generic interface and I
> > don't think it makes sense to tie read-specific data to it. For
> > example, some filesystems (eg gfs2) use the same iomap_ops across
> > different file operations (eg buffered writes, direct io, reads, bmap,
> > etc).
>
> Previously `.iomap_{begin,end}` participates in buffer read and write
> I/O paths (except for page writeback of course) as you said, in
> principle users only need to care about fields in `struct iomap_iter`.
>
> `struct iomap_readpage_ctx` is currently used as an internal structure
> which is completely invisible to filesystems (IOWs, filesystems don't
> need to care or specify any of that).
>
> After your proposal, new renamed `struct iomap_read_folio_ctx` will be
> exposed to individual filesystems too, but that makes two external
> context structures for the buffer I/O reads (`struct iomap_iter` and
> `struct iomap_read_folio_ctx`) instead of one.
>
> I'm not saying your proposal doesn't work, but:
>
> - which is unlike `struct iomap_writepage_ctx` because writeback path
> doesn't have `struct iomap_iter` involved, and it has only that
> exact one `struct iomap_writepage_ctx` context and all callbacks
> use that only;
>
> - take a look at `iomap_dio_rw` and `iomap_dio_ops`, I think it's
> somewhat similiar to the new `struct iomap_read_ops` in some
> extent, but dio currently also exposes the exact one context
> (`struct iomap_iter`) to users.
>
> - take a look at `iomap_write_ops`, it also exposes
> `struct iomap_iter` only. you may say `folio`, `pos`, `len` can be
> wrapped as another `struct iomap_write_ctx` if needed, but that is
> not designed to be exposed to be specfied by write_iter (e.g.
> fuse_cache_write_iter)
>
> In short, traditionally the buffered read/write external context is
> the only unique one `struct iomap_iter` (`struct iomap_readpage_ctx`
> is only for iomap internal use), after your proposal there will be
> two external contexts specified by users (.read_folio and .readahead)
> but `.iomap_{begin,end}` is unable to get one of them, which is
> unlike the current writeback and direct i/o paths (they uses one
> external context too.)
>
> Seperate into two contexts works for your use case, but it may
> cause issues since future developers have to decide where to
> place those new context fields for buffer I/O paths (
> `struct iomap_iter` or `struct iomap_read_folio_ctx`), it's still
> possible but may cause further churn on the codebase perspective.
>
> That is my minor concern, but my main concern is still `private`
> naming.
Hi Gao,
In my mind, the big question is whether or not the data the
filesystems pass in is logically shared by both iomap_begin/end and
buffered reads/writes/dio callbacks, or whether the data needed by
both are basically separate entities but have to be frankensteined
together so that it can be passed in through iter->private. My sense
of the read/readahead code is that the data needed by iomap begin/end
vs buffered reads are basically logically separate entities. I see
your point about how the existing code for buffered writes and dio in
iomap have them combined into one, but imo, if the iomap_iter data is
a separate entity from the data needed in the callbacks, then those
pointers should be separate.
But I also am happy to change this back to having it the way it was
for v1 where everything just went through iter->private. I don't feel
strongly about this decision, I'm happy with whichever way we go with.
Thanks,
Joanne
>
> Thanks,
> Gao Xiang
>
> > Thanks,
> > Joanne
> >
> >>
> >> Thanks,
> >> Gao Xiang
>
