Re: [PATCH v5 4/4] iomap: add simple dio path for small direct I/O

[changfengnan]

> From: "Christoph Hellwig"<hch@xxxxxxxxxxxxx>
> Date:  Tue, Jun 30, 2026, 19:49
> Subject:  Re: [PATCH v5 4/4] iomap: add simple dio path for small direct I/O
> To: "Fengnan Chang"<changfengnan@xxxxxxxxxxxxx>
> Cc: <brauner@xxxxxxxxxx>, <djwong@xxxxxxxxxx>, <hch@xxxxxxxxxxxxx>, <ojaswin@xxxxxxxxxxxxx>, <dgc@xxxxxxxxxx>, <linux-xfs@xxxxxxxxxxxxxxx>, <linux-fsdevel@xxxxxxxxxxxxxxx>, <linux-ext4@xxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>, <lidiangang@xxxxxxxxxxxxx>, <pankaj.raghav@xxxxxxxxx>
> On Mon, Jun 29, 2026 at 08:01:24PM +0800, Fengnan Chang wrote:
> > +#include <linux/kobject.h>
> > +#include <linux/sysfs.h>
> 
> I don't think these two headers are actually used by the new code.

Debug code, will delete in next version.

> 
> > +struct iomap_dio_simple {
> > +        struct kiocb                *iocb;
> > +        size_t                        size;
> > +        unsigned int                dio_flags;
> > +        struct work_struct        work;
> > +        /*
> > +         * Align @bio to a cacheline boundary so that, combined with the
> > +         * front_pad passed to bioset_init(), the bio sits at the start of
> > +         * a cacheline in memory returned by the (HWCACHE-aligned) bio
> > +         * slab.  This keeps the hot fields block layer touches on submit
> > +         * and completion (bi_iter, bi_status, ...) within a single line.
> > +         */
> > +        struct bio        bio ____cacheline_aligned_in_smp;
> 
> Add an extra tab here so it aligns with the fields above the comment?

All comments will fix in next version, thanks.

> 
> > +        int error = blk_status_to_errno(bio->bi_status);
> > +        ssize_t ret = error;
> > +
> > +        if (likely(!ret)) {
> > +                ret = sr->size;
> > +                iocb->ki_pos += ret;
> > +        } else if (should_report_dio_fserror(ret)) {
> > +                fserror_report_io(inode, FSERR_DIRECTIO_READ, iocb->ki_pos,
> > +                                  sr->size, ret, GFP_NOFS);
> > +        }
> > +
> > +        iomap_dio_bio_release_pages(bio, sr->dio_flags, ret < 0);
> > +        inode_dio_end(inode);
> > +        trace_iomap_dio_complete(iocb, error, ret);
> 
> I think some of this could be micro-optimized a bit more and
> cleaned up at the same time:
> 
>         ssize_t ret;
> 
>         if (unlikely(bio->bi_status) {
>                 ret = blk_status_to_errno(bio->bi_status);
>                 if (should_report_dio_fserror(ret)) {
>                         fserror_report_io(inode, FSERR_DIRECTIO_READ,
>                                         iocb->ki_pos, sr->size, ret,
>                                         GFP_NOFS);
>                 }
>         } else {
>                 ret = sr->size;
>                 iocb->ki_pos += ret;
>         }
> 
>         iomap_dio_bio_release_pages(bio, sr->dio_flags, ret < 0);
>         inode_dio_end(inode);
>         trace_iomap_dio_complete(iocb, ret < 0 ? ret : 0, ret);
> 
>         return ret;
> 
> > +
> > +static void iomap_dio_simple_complete_work(struct work_struct *work)
> > +{
> > +        struct iomap_dio_simple *sr =
> > +                        container_of(work, struct iomap_dio_simple, work);
> 
> A single tab indent is enough here. (and used in the next function)
> 
> > +        struct bio *bio;
> > +        bool wait_for_completion = is_sync_kiocb(iocb);
> > +        ssize_t ret;
> 
> Nitpick: keep variables initialized at declaration time above those
> that are not initalized.
> 
> > +        /*
> > +         * Fast path for small, block-aligned direct I/Os that map to a
> > +         * single contiguous on-disk extent.
> > +         *
> > +         * @dops must be NULL: a non-NULL @dops means the caller wants its
> > +         * ->end_io / ->submit_io hooks invoked, and in particular wants its
> > +         * bios to be allocated from the filesystem-private @dops->bio_set
> > +         * (whose front_pad sizes a filesystem-private wrapper around the
> > +         * bio).  The fast path instead allocates from the shared
> > +         * iomap_dio_simple_pool, whose front_pad matches
> > +         * struct iomap_dio_simple; the two wrappers are not
> > +         * interchangeable, so we must fall back to __iomap_dio_rw() in
> > +         * that case.
> > +         *
> > +         * @done_before must be zero: a non-zero caller-accumulated residual
> > +         * cannot be carried through a single-bio inline completion.
> > +         *
> > +         * -ENOTBLK is the private sentinel returned by iomap_dio_simple()
> > +         * when it decides the request does not fit the fast path.
> > +         * In that case we proceed to the generic __iomap_dio_rw() slow
> > +         * path.  Any other errno is a real result and is propagated as-is,
> > +         * in particular -EAGAIN for IOCB_NOWAIT must reach the caller.
> 
> I think this documentation belongs above iomap_dio_simple, not
> here.  And maye it should also mention that iomap_dio_simple_supported
> enforces these limitations.  Mostly anyway, and maybe the other two
> checks should go into that as well for completeness?
>