On Mon, Sep 8, 2025 at 8:14 PM Gao Xiang [off-list ref] wrote:

Hi Joanne,

On Mon, Sep 08, 2025 at 11:51:17AM -0700, Joanne Koong wrote:

quoted

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

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

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


Thanks,
Joanne

Thanks,
Gao Xiang