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

Gao Xiang
2025-09-11T12:29:29Z

Hi Christoph,

On 2025/9/11 19:37, Christoph Hellwig wrote:
> On Wed, Sep 10, 2025 at 12:59:41PM +0800, Gao Xiang wrote:
>> 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.
```

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

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