Re: [PATCH 1/4] iomap: add IOMAP_DIO_FSBLOCK_ALIGNED flag

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Oct 28, 2025 at 08:11:21AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2025/10/28 02:40, Darrick J. Wong 写道:
> > On Thu, Oct 23, 2025 at 03:55:42PM +0200, Christoph Hellwig wrote:
> > > From: Qu Wenruo <wqu@suse.com>
> > > 
> > > Btrfs requires all of its bios to be fs block aligned, normally it's
> > > totally fine but with the incoming block size larger than page size
> > > (bs > ps) support, the requirement is no longer met for direct IOs.
> > > 
> > > Because iomap_dio_bio_iter() calls bio_iov_iter_get_pages(), only
> > > requiring alignment to be bdev_logical_block_size().
> > > 
> > > In the real world that value is either 512 or 4K, on 4K page sized
> > > systems it means bio_iov_iter_get_pages() can break the bio at any page
> > > boundary, breaking btrfs' requirement for bs > ps cases.
> > > 
> > > To address this problem, introduce a new public iomap dio flag,
> > > IOMAP_DIO_FSBLOCK_ALIGNED.
> > > 
> > > When calling __iomap_dio_rw() with that new flag, iomap_dio::flags will
> > > inherit that new flag, and iomap_dio_bio_iter() will take fs block size
> > > into the calculation of the alignment, and pass the alignment to
> > > bio_iov_iter_get_pages(), respecting the fs block size requirement.
> > > 
> > > The initial user of this flag will be btrfs, which needs to calculate the
> > > checksum for direct read and thus requires the biovec to be fs block
> > > aligned for the incoming bs > ps support.
> > > 
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >   fs/iomap/direct-io.c  | 13 ++++++++++++-
> > >   include/linux/iomap.h |  8 ++++++++
> > >   2 files changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > > index 5d5d63efbd57..ce9cbd2bace0 100644
> > > --- a/fs/iomap/direct-io.c
> > > +++ b/fs/iomap/direct-io.c
> > > @@ -336,10 +336,18 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
> > >   	int nr_pages, ret = 0;
> > >   	u64 copied = 0;
> > >   	size_t orig_count;
> > > +	unsigned int alignment = bdev_logical_block_size(iomap->bdev);
> > >   	if ((pos | length) & (bdev_logical_block_size(iomap->bdev) - 1))
> > >   		return -EINVAL;
> > > +	/*
> > > +	 * Align to the larger one of bdev and fs block size, to meet the
> > > +	 * alignment requirement of both layers.
> > > +	 */
> > > +	if (dio->flags & IOMAP_DIO_FSBLOCK_ALIGNED)
> > > +		alignment = max(alignment, fs_block_size);
> > > +
> > >   	if (dio->flags & IOMAP_DIO_WRITE) {
> > >   		bio_opf |= REQ_OP_WRITE;
> > > @@ -434,7 +442,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
> > >   		bio->bi_end_io = iomap_dio_bio_end_io;
> > >   		ret = bio_iov_iter_get_pages(bio, dio->submit.iter,
> > > -				bdev_logical_block_size(iomap->bdev) - 1);
> > > +					     alignment - 1);
> > >   		if (unlikely(ret)) {
> > >   			/*
> > >   			 * We have to stop part way through an IO. We must fall
> > > @@ -639,6 +647,9 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
> > >   	if (iocb->ki_flags & IOCB_NOWAIT)
> > >   		iomi.flags |= IOMAP_NOWAIT;
> > > +	if (dio_flags & IOMAP_DIO_FSBLOCK_ALIGNED)
> > > +		dio->flags |= IOMAP_DIO_FSBLOCK_ALIGNED;
> > > +
> > >   	if (iov_iter_rw(iter) == READ) {
> > >   		/* reads can always complete inline */
> > >   		dio->flags |= IOMAP_DIO_INLINE_COMP;
> > > diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> > > index 73dceabc21c8..4da13fe24ce8 100644
> > > --- a/include/linux/iomap.h
> > > +++ b/include/linux/iomap.h
> > > @@ -518,6 +518,14 @@ struct iomap_dio_ops {
> > >    */
> > >   #define IOMAP_DIO_PARTIAL		(1 << 2)
> > > +/*
> > > + * Ensure each bio is aligned to fs block size.
> > > + *
> > > + * For filesystems which need to calculate/verify the checksum of each fs
> > > + * block. Otherwise they may not be able to handle unaligned bios.
> > > + */
> > > +#define IOMAP_DIO_FSBLOCK_ALIGNED	(1 << 3)
> > 
> > A new flag requires an update to IOMAP_F_FLAGS_STRINGS in trace.h for
> > tracepoint decoding.
> 
> I'm wondering who should fix this part.
> 
> The original author (myself) or Christoph?

Or the maintainer?

Here's the relevant patch for whomever actually commits it.
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>

diff --git i/fs/iomap/trace.h w/fs/iomap/trace.h
index a61c1dae474270..eae9c1e5c674c8 100644
--- i/fs/iomap/trace.h
+++ w/fs/iomap/trace.h
@@ -121,13 +121,14 @@ DEFINE_RANGE_EVENT(iomap_zero_iter);
        { IOMAP_F_STALE,        "STALE" }
 
 
 #define IOMAP_DIO_STRINGS \
        {IOMAP_DIO_FORCE_WAIT,  "DIO_FORCE_WAIT" }, \
        {IOMAP_DIO_OVERWRITE_ONLY, "DIO_OVERWRITE_ONLY" }, \
-       {IOMAP_DIO_PARTIAL,     "DIO_PARTIAL" }
+       {IOMAP_DIO_PARTIAL,     "DIO_PARTIAL" }, \
+       {IOMAP_DIO_FSBLOCK_ALIGNED,     "DIO_FSBLOCK" }
 
 DECLARE_EVENT_CLASS(iomap_class,
        TP_PROTO(struct inode *inode, struct iomap *iomap),
        TP_ARGS(inode, iomap),
        TP_STRUCT__entry(
                __field(dev_t, dev)

> Thanks,
> Qu
> 
> > 
> > The rest of the changes look ok to me, modulo hch's subsequent fixups.
> > 
> > --D
> > 
> > >   ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
> > >   		const struct iomap_ops *ops, const struct iomap_dio_ops *dops,
> > >   		unsigned int dio_flags, void *private, size_t done_before);
> > > -- 
> > > 2.47.3
> > > 
> > > 
>