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

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.

Thanks,
Joanne

> +       return ret;
> +}