Re: [PATCH 2/3] iomap: decouple simple direct I/O reads from iomap_dio_rw

> From: "Joanne Koong"<[email protected]>
> Date:  Thu, Jul 23, 2026, 03:04
> Subject:  Re: [PATCH 2/3] iomap: decouple simple direct I/O reads from iomap_dio_rw
> To: "Christoph Hellwig"<[email protected]>
> Cc: <[email protected]>, "Darrick J. Wong"<[email protected]>, "Christian Brauner"<[email protected]>, "Theodore Ts'o"<[email protected]>, "Carlos Maiolino"<[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>
> On Wed, Jul 22, 2026 at 5:49 AM Christoph Hellwig <[email protected]> wrote:
> >
> > The pending iomap_iter_next conversion creates performance issues for the
> > new simple direct I/O read fast path, because it assumes a model where
> > the iterator must be advanced at the end, which the direct I/O read fast
> > path tries to avoid.
> >
> > Side step this by splitting the simple path from iomap_dio_rw, and
> > require the file systems to call into it explicitly, and pass only a
> > ->begin callback.  This allows to drop various checks for incompatible
> > features while creating a requirement for the file system to only call
> > the simple path for cases that it can handle.
> 
> Nice, this approach looks cleaner to me.
> 
> >
> > As a side-benefit we can now inline the initial part of the simple
> > direct I/O read fast path and let the compiler convert the indirect call
> > to ->begin into a direct call.
> >
> > Signed-off-by: Christoph Hellwig <[email protected]>
> > ---
> >  fs/ext4/ext4.h        |   3 +
> >  fs/ext4/file.c        |   4 +-
> >  fs/ext4/inode.c       |   2 +-
> >  fs/iomap/direct-io.c  | 209 ++++++++++--------------------------------
> >  fs/xfs/xfs_file.c     |  14 +--
> >  fs/xfs/xfs_iomap.c    |   2 +-
> >  fs/xfs/xfs_iomap.h    |   4 +
> >  include/linux/iomap.h |  65 +++++++++++++
> >  8 files changed, 133 insertions(+), 170 deletions(-)
> >
> > index 3df851d0f76b..810dc4b8dc94 100644
> > --- a/include/linux/iomap.h
> > +++ b/include/linux/iomap.h
> >
> > +/*
> > + * Fast path for small, block-aligned direct I/Os that map to a single
> > + * contiguous on-disk extent.
> > + *
> > + * @iter must describe a non-empty READ no larger than the inode block size:
> > + * writes, zero-length I/O, and larger requests need the generic iomap direct
> > + * I/O path.
> > + *
> > + * Does not support iomap_dio_ops, dio_flags, done_before or private data.
> > + * The range must also stay within i_size and encrypted inodes must use the
> > + * generic iomap direct I/O path.
> > + *
> > + * -ENOTBLK inidicates the generic path must be used by the caller instead.
> > + * Any other errno is a real result and is propagated as-is, in particular
> > + * -EAGAIN for IOCB_NOWAIT must reach the caller.
> > + *
> > + * The caller can only provide an iomap begin handler, and the iterator
> > + * is never advanced.
> > + */
> > +ssize_t __iomap_dio_simple(struct kiocb *iocb, struct iov_iter *iter,
> > +               struct iomap_iter *iomi);
> 
> Might be nice to have _read_ in this name as well, eg __iomap_dio_read_simple
> 
> > +static __always_inline ssize_t iomap_dio_read_simple(struct kiocb *iocb,
> > +               struct iov_iter *iter, iomap_iter_begin_fn begin)
> > +{
> > +       struct iomap_iter iomi = {
> > +               .inode          = file_inode(iocb->ki_filp),
> > +               .pos            = iocb->ki_pos,
> > +               .len            = iov_iter_count(iter),
> > +               .flags          = IOMAP_DIRECT,
> > +       };
> > +       ssize_t ret;
> > +
> > +       if (!iomi.len)
> > +               return 0;
> > +
> > +       /*
> > +        * Simple dio is an optimization for small IO. Filter out large IO
> > +        * early as it's the most common case to fail for typical direct IO
> > +        * workloads.
> > +        */
> > +       if (iomi.len > iomi.inode->i_sb->s_blocksize)
> > +               return -ENOTBLK;
> > +       if (iocb->ki_pos + iomi.len > i_size_read(iomi.inode))
> > +               return -ENOTBLK;
> > +       if (IS_ENCRYPTED(iomi.inode))
> > +               return -ENOTBLK;
> > +
> > +       ret = kiocb_write_and_wait(iocb, iomi.len);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (iocb->ki_flags & IOCB_NOWAIT)
> > +               iomi.flags |= IOMAP_NOWAIT;
> > +
> > +       inode_dio_begin(iomi.inode);
> > +       ret = begin(iomi.inode, iomi.pos, iomi.len, iomi.flags, &iomi.iomap,
> > +                       &iomi.srcmap);
> > +       if (!ret)
> > +               ret = __iomap_dio_simple(iocb, iter, &iomi);
> > +       if (ret <= 0 && ret != -EIOCBQUEUED)
> > +               inode_dio_end(iomi.inode);
> 
> afaict, the inode_dio_end() error handling has to be done by
> __iomap_dio_simple() to properly account for the synchronous case. I
> ran into this earlier when doing the iomap_next_fn inlining. For the
> synchronous case, __iomap_dio_simple() calls
> iomap_dio_simple_complete() which calls inode_dio_end() even on
> errors.

Yes, there is an issue with the handling of the inode_dio_end event in the
synchronization scenario.
How about this ?

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 3bdfbd0efa076..f6c8091a65ac9 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -994,18 +994,22 @@ ssize_t __iomap_dio_simple(struct kiocb *iocb, struct iov_iter *iter,
 
         if (iomi->iomap.type != IOMAP_MAPPED ||
             iomi->iomap.offset + iomi->iomap.length < iomi->pos + iomi->len ||
-            (iomi->iomap.flags & IOMAP_F_INTEGRITY))
-                return -ENOTBLK;
+            (iomi->iomap.flags & IOMAP_F_INTEGRITY)) {
+                ret = -ENOTBLK;
+                goto out_dio_end;
+        }
 
         alignment = iomap_dio_alignment(iomi->inode, iomi->iomap.bdev, 0);
-        if ((iomi->pos | iomi->len) & (alignment - 1))
-                return -EINVAL;
+        if ((iomi->pos | iomi->len) & (alignment - 1)) {
+                ret = -EINVAL;
+                goto out_dio_end;
+        }
 
         if (unlikely(!iomi->inode->i_sb->s_dio_done_wq &&
                         !is_sync_kiocb(iocb))) {
                 ret = sb_init_dio_done_wq(iomi->inode->i_sb);
                 if (ret < 0)
-                        return ret;
+                        goto out_dio_end;
         }
 
         trace_iomap_dio_rw_begin(iocb, iter, 0, 0);
@@ -1013,8 +1017,10 @@ ssize_t __iomap_dio_simple(struct kiocb *iocb, struct iov_iter *iter,
         bio = bio_alloc_bioset(iomi->iomap.bdev,
                                bio_iov_vecs_to_alloc(iter, BIO_MAX_VECS),
                                REQ_OP_READ, gfp, &iomap_dio_simple_pool);
-        if (!bio)
-                return -EAGAIN;
+        if (!bio) {
+                ret = -EAGAIN;
+                goto out_dio_end;
+        }
         sr = container_of(bio, struct iomap_dio_simple, bio);
         sr->iocb = iocb;
         sr->dio_flags = 0;
@@ -1060,6 +1066,8 @@ ssize_t __iomap_dio_simple(struct kiocb *iocb, struct iov_iter *iter,
         bio_release_pages(bio, false);
 out_bio_put:
         bio_put(bio);
+out_dio_end:
+        inode_dio_end(iomi->inode);
         return ret;
 }
 EXPORT_SYMBOL_GPL(__iomap_dio_simple);
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 810dc4b8dc94d..3cdf475059048 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -732,11 +732,11 @@ static __always_inline ssize_t iomap_dio_read_simple(struct kiocb *iocb,
         inode_dio_begin(iomi.inode);
         ret = begin(iomi.inode, iomi.pos, iomi.len, iomi.flags, &iomi.iomap,
                         &iomi.srcmap);
-        if (!ret)
-                ret = __iomap_dio_simple(iocb, iter, &iomi);
-        if (ret <= 0 && ret != -EIOCBQUEUED)
+        if (ret) {
                 inode_dio_end(iomi.inode);
-        return ret;
+                return ret;
+        }
+        return __iomap_dio_simple(iocb, iter, &iomi);
 }
 
 #ifdef CONFIG_SWAP
> 
> Thanks,
> Joanne
> 
> > +       return ret;
> > +}
>