On Tue, Sep 23, 2025 at 5:26 PM Darrick J. Wong [off-list ref] wrote:

On Mon, Sep 22, 2025 at 05:23:47PM -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 *read_ctx;
  };

I'm starting to wonder if struct iomap_read_ops should contain a struct
iomap_ops object, but that might result in more churn through this
patchset.

<shrug> What do you think?

Lol I thought the same thing a while back for "struct iomap_write_ops"
but I don't think Christoph liked the idea [1]

[1] https://lore.kernel.org/linux-fsdevel/20250618044344.GE28041@lst.de/ (local)

quoted

  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,
                             size_t len);
      void (*read_submit)(struct iomap_read_folio_ctx *ctx);
  };

  read_folio_range() reads in the folio range and is required by the
  caller to provide. read_submit() is optional and is used for
  submitting any pending read requests.

* Modifies existing filesystems that use iomap for read and readahead to
  use the new API, through the new statically inlined helpers
  iomap_bio_read_folio() and iomap_bio_readahead(). There is no change
  in functinality for those filesystems.

Nit: functionality

Thanks, will fix this!

quoted

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 .../filesystems/iomap/operations.rst          | 45 ++++++++++++
 block/fops.c                                  |  5 +-
 fs/erofs/data.c                               |  5 +-
 fs/gfs2/aops.c                                |  6 +-
 fs/iomap/buffered-io.c                        | 68 +++++++++++--------
 fs/xfs/xfs_aops.c                             |  5 +-
 fs/zonefs/file.c                              |  5 +-
 include/linux/iomap.h                         | 62 ++++++++++++++++-
 8 files changed, 158 insertions(+), 43 deletions(-)

diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst
index 067ed8e14ef3..dbb193415c0e 100644
--- a/Documentation/filesystems/iomap/operations.rst
+++ b/Documentation/filesystems/iomap/operations.rst

@@ -135,6 +135,29 @@ These ``struct kiocb`` flags are significant for buffered I/O with iomap:

  * ``IOCB_DONTCACHE``: Turns on ``IOMAP_DONTCACHE``.

+``struct iomap_read_ops``
+--------------------------
+
+.. code-block:: c
+
+ struct iomap_read_ops {
+     int (*read_folio_range)(const struct iomap_iter *iter,
+                             struct iomap_read_folio_ctx *ctx, size_t len);
+     void (*submit_read)(struct iomap_read_folio_ctx *ctx);
+ };
+
+iomap calls these functions:
+
+  - ``read_folio_range``: Called to read in the range. This must be provided
+    by the caller. The caller is responsible for calling
+    iomap_start_folio_read() and iomap_finish_folio_read() before and after
+    reading in the folio range. This should be done even if an error is
+    encountered during the read. This returns 0 on success or a negative error
+    on failure.
+
+  - ``submit_read``: Submit any pending read requests. This function is
+    optional.
+
 Internal per-Folio State
 ------------------------

@@ -182,6 +205,28 @@ The ``flags`` argument to ``->iomap_begin`` will be set to zero.
 The pagecache takes whatever locks it needs before calling the
 filesystem.

+Both ``iomap_readahead`` and ``iomap_read_folio`` pass in a ``struct
+iomap_read_folio_ctx``:
+
+.. code-block:: c
+
+ struct iomap_read_folio_ctx {
+    const struct iomap_read_ops *ops;
+    struct folio *cur_folio;
+    struct readahead_control *rac;
+    void *read_ctx;
+ };
+
+``iomap_readahead`` must set:
+ * ``ops->read_folio_range()`` and ``rac``
+
+``iomap_read_folio`` must set:
+ * ``ops->read_folio_range()`` and ``cur_folio``

Hrmm, so we're multiplexing read and readahead through the same
iomap_read_folio_ctx.  Is there ever a case where cur_folio and rac can
both be used by the underlying machinery?  I think the answer to that
question is "no" but I don't think the struct definition makes that
obvious.

In the ->read_folio_range() callback, both rac and cur_folio are used
for readahead, but in passing in the "struct iomap_read_folio_ctx" to
the main iomap_read_folio()/iomap_readahead() entrypoint, no both rac
and cur_folio do not get set at the same time.

We could change the signature back to something like:
int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops,
const struct iomap_read_ops *ops, void *read_ctx);
void iomap_readahead(struct readahead_control *rac, const struct
iomap_ops *ops, const struct iomap_read_ops *ops, void *read_ctx);

but I think it might get a bit much if/when "void *private" needs to
get added too for iomap iter metadata, though maybe that's okay now
that the private read data has been renamed to read_ctx.

quoted

+
 static int iomap_read_folio_iter(struct iomap_iter *iter,
              struct iomap_read_folio_ctx *ctx, bool *folio_owned)
 {

@@ -436,7 +438,7 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
      loff_t length = iomap_length(iter);
      struct folio *folio = ctx->cur_folio;
      size_t poff, plen;
-     loff_t count;
+     loff_t pos_diff;
      int ret;

      if (iomap->type == IOMAP_INLINE) {

@@ -454,12 +456,16 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
              iomap_adjust_read_range(iter->inode, folio, &pos, length, &poff,
                              &plen);

-             count = pos - iter->pos + plen;
-             if (WARN_ON_ONCE(count > length))
+             pos_diff = pos - iter->pos;
+             if (WARN_ON_ONCE(pos_diff + plen > length))
                      return -EIO;

Er, can these changes get their own patch describing why the count ->
pos_diff change was made?

I will separate this out into its own patch. The reasoning behind this
is so that the ->read_folio_range() callback doesn't need to take in a
pos arg but instead can get it from iter->pos [1]

[1] https://lore.kernel.org/linux-fsdevel/aMKt52YxKi1Wrw4y@infradead.org/ (local)

Thanks for looking at this patchset!

Thanks,
Joanne

--D