On Tue, Sep 09, 2025 at 08:14:49AM +0800, Gao Xiang 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);
>
btw, if iomap folks really think it looks clean to pass in two
different `private` like this, I'm fine, basically:
I need a way to create an on-stack context in `erofs_read_folio()`
and `erofs_readahead()` and pass it down to .iomap_{begin,end}
because the current `.iomap_begin` and `.iomap_end` has no way to
get the new on-stack context: it can only get inode,pos,len,etc.
As Darrick mentioned, `iter = container_of(iomap)` usage in
`xfs_zoned_buffered_write_iomap_begin()` and
`xfs_buffered_write_delalloc_punch()` looks uneasy to me as well,
because it couples `struct iomap *` and `struct iomap_iter *` with
iomap implementation internals: At least `struct iomap_iter` has
two `struct iomap`, without any details, it's hard to assume it's
the `iter->iomap` one.
> 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.
>
> Thanks,
> Gao Xiang
