Re: [PATCH v2 11/16] iomap: add caller-provided callbacks for read and readahead

[Gao Xiang <hsiangkao@linux.alibaba.com>]

Hi Joanne,

On 2025/9/9 23:24, Joanne Koong wrote:
> On Mon, Sep 8, 2025 at 8:14 PM Gao Xiang <xiang@kernel.org> 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.

Thanks,
Gao Xiang


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