Re: [PATCH v2 13/16] iomap: move read/readahead logic out of CONFIG_BLOCK guard

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

On 2025/9/13 03:56, Joanne Koong wrote:
> On Thu, Sep 11, 2025 at 9:11 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>>
>> On 2025/9/12 09:09, Gao Xiang wrote:
>>>
>>>
>>> On 2025/9/12 08:06, Gao Xiang wrote:
>>>>
>>>>
>>>> On 2025/9/12 03:45, Joanne Koong wrote:
>>>>> On Thu, Sep 11, 2025 at 8:29 AM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>>>>
>>>>>> But if FUSE or some other fs later needs to request L2P information
>>>>>> in their .iomap_begin() and need to send L2P requests to userspace
>>>>>> daemon to confirm where to get the physical data (maybe somewhat
>>>>>> like Darrick's work but I don't have extra time to dig into that
>>>>>> either) rather than just something totally bypass iomap-L2P logic
>>>>>> as above, then I'm not sure the current `iomap_iter->private` is
>>>>>> quite seperate to `struct iomap_read_folio_ctx->private`, it seems
>>>>>
>>>>> If in the future this case arises, the L2P mapping info is accessible
>>>>> by the read callback in the current design. `.read_folio_range()`
>>>>> passes the iomap iter to the filesystem and they can access
>>>>> iter->private to get the L2P mapping data they need.
>>>>
>>>> The question is what exposes to `iter->private` then, take
>>>> an example:
>>>>
>>>> ```
>>>> struct file *file;
>>>> ```
>>>>
>>>> your .read_folio_range() needs `file->private_data` to get
>>>> `struct fuse_file` so `file` is kept into
>>>> `struct iomap_read_folio_ctx`.
>>>>
>>>> If `file->private_data` will be used for `.iomap_begin()`
>>>> as well, what's your proposal then?
>>>>
>>>> Duplicate the same `file` pointer in both
>>>> `struct iomap_read_folio_ctx` and `iter->private` context?
>>>
>>> It's just an not-so-appropriate example because
>>> `struct file *` and `struct fuse_file *` are widely used
>>> in the (buffer/direct) read/write flow but Darrick's work
>>> doesn't use `file` in .iomap_{begin/end}.
>>>
>>> But you may find out `file` pointer is already used for
>>> both FUSE buffer write and your proposal, e.g.
>>>
>>> buffer write:
>>>    /*
>>>     * Use iomap so that we can do granular uptodate reads
>>>     * and granular dirty tracking for large folios.
>>>     */
>>>    written = iomap_file_buffered_write(iocb, from,
>>>                                        &fuse_iomap_ops,
>>>                                        &fuse_iomap_write_ops,
>>>                                        file);
>>
>> And your buffer write per-fs context seems just use
>> `iter->private` entirely instead to keep `file`.
>>
> 
> I don’t think the iomap buffered writes interface is good to use as a
> model. I looked a bit at some of the other iomap file operations and I
> think we should just pass operation-specific data through an
> operation-specific context for those too, eg for buffered writes and
> dio modifying the interface from
> 
> ssize_t iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter
> *from, const struct iomap_ops *ops, const struct iomap_write_ops
> *write_ops, void *private);
> ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, const
> struct iomap_ops *ops, const struct iomap_dio_ops *dops, unsigned int
> dio_flags, void *private, size_t done_before);
> 
> to something like
> 
> ssize_t iomap_file_buffered_write(const struct iomap_ops *ops, struct
> iomap_write_folio_ctx *ctx);
> ssize_t iomap_dio_rw(const struct iomap_ops *ops, struct iomap_dio_ctx *ctx);
> 
> There’s one filesystem besides fuse that uses “iter->private” and
> that’s for xfs zoned inodes (xfs_zoned_buffered_write_iomap_begin()),
> which passes the  struct xfs_zone_alloc_ctx*  through iter->private,
> and it's used afaict for tracking block reservations. imo that's what
> iter->private should be used for, to track the more high level
> metadata stuff and then the lower-level details that are
> operation-specific go through the ctx->data fields. That seems the
> cleanest design to me. I think we should rename the iter->private
> field to something like "iter->metadata" to make that delineation more
> clear.  I'm not sure what the iomap maintainers think, but that is my
> opinion.

In short, I don't think new "low-level" and "high-level" concepts are
really useful even for disk fses.

> 
> I think if in the future there is a case/feature which needs something
> previously in one of the operation-specific ctxes, it seems fine to me
> to have both iter->private and ctx->data point to the same thing.
> 

I want to stop this topic here, it's totally up to iomap maintainers to
decide what's the future iomap looks like but I still keep my strong
reserve opinion (you can ignore) from my own code design taste.

Thanks,
Gao Xiang

> 
> Thanks,
> Joanne