On Thu, Sep 11, 2025 at 8:29 AM Gao Xiang [off-list ref] wrote:

Hi Christoph,

On 2025/9/11 19:37, Christoph Hellwig wrote:

quoted

On Wed, Sep 10, 2025 at 12:59:41PM +0800, Gao Xiang wrote:

quoted

At least it sounds better on my side, but anyway it's just
my own overall thought.  If other folks have different idea,
I don't have strong opinion, I just need something for my own
as previous said.

I already dropped my two suggestions on the earlier patch.  Not totally
happy about either my suggestions or data, but in full agreement that
it should be something else than private.

To just quote your previous comment and try to discuss here:

On Wed, Sep 10, 2025 at 01:41:25PM -0400, Joanne Koong wrote:
> 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

They are separate entities.

Hi Gao,

I try to push this again because I'm still not quite sure it's
a good idea, let's take this FUSE iomap-read proposal (but sorry
honestly I not fully look into the whole series.)

  struct fuse_fill_read_data {
        struct file *file;
+
+       /*
+        * Fields below are used if sending the read request
+        * asynchronously.
+        */
+       struct fuse_conn *fc;
+       struct fuse_io_args *ia;
+       unsigned int nr_bytes;
  };

which is just a new FUSE-only-specific context for
`struct iomap_read_folio_ctx`, it's not used by .iomap_{begin,end}
is that basically FUSE _currently_ doesn't have logical-to-physical
mapping requirement (except for another fuse_iomap_begin in dax.c):

I don't think this is true. The other filesystems in the kernel using
iomap that do need logical to physical mappings also do not have their
context for `struct iomap_read_folio_ctx` (the struct bio) used by
.iomap_{begin, end} either. As I see it, the purpose of the `struct
iomap_read_folio_ctx` context is for processing/issuing the reads and
the context for .iomap_{begin,end} is for doing all the mapping /
general metadata tracking stuff - even for the filesystems that have
the logical to physical mapping requirements, their usage of the
context is for processing/submitting the bio read requests, which imo
the more high-level iomap_{begin,end} is a layer above.

static int fuse_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
                             unsigned int flags, struct iomap *iomap,
                             struct iomap *srcmap)
{
         iomap->type = IOMAP_MAPPED;
         iomap->length = length;
         iomap->offset = offset;
         return 0;
}

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.

both needs fs-specific extra contexts for the same I/O flow.

I think the reason why `struct iomap_read_folio_ctx->private` is
introduced is basically previous iomap filesystems are all
bio-based, and they shares `bio` concept in common but
`iter->private` was not designed for this usage.

But fuse `struct iomap_read_folio_ctx` and
`struct fuse_fill_read_data` are too FUSE-specific, I cannot
see it could be shared by other filesystems in the near future,
which is much like a single-filesystem specific concept, and
unlike to `bio` at all.

Currently fuse is the only non-block-based filesystem using iomap but
I don't see why there wouldn't be more in the future. For example,
while looking at some of the netfs code, a lot of the core
functionality looks the same between that and iomap and I think it
might be a good idea to have netfs in the future use iomap's interface
so that it can get the large folio dirty/uptodate tracking stuff and
any other large folio stuff like more granular writeback stats
accounting for free.


Thanks,
Joanne

I've already racked my brains on this but I have no better
idea on the current callback-hook model (so I don't want to argue
more). Anyway, I really think it should be carefully designed
(because the current FUSE .iomap_{begin,end} are just like no-op
but that is just fuse-specific).  If folks really think Joanne's
work is already best or we can live with that, I'm totally fine.

Thanks,
Gao Xiang