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

[Christoph Hellwig]

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.

> +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?

> +	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?