[PATCH] iomap: add IOMAP_DIO_ALIGNED flag for btrfs

[PATCH] iomap: add IOMAP_DIO_ALIGNED flag for btrfs

[Qu Wenruo]

With the recent bs > ps support for btrfs, btrfs requires block
alignment for all of its bios.

However the current iomap_dio_bio_iter() calls bio_iov_iter_get_pages()
which only ensure alignment to bdev_logical_block_size().

In the real world it's mostly 512 or 4K, resulting some bio to be split
at page boundary, breaking the btrfs requirement.

To address this problem, introduce a new public iomap dio flag,
IOMAP_DIO_ALIGNED.

When calling __iomap_dio_rw() with that new flag, iomap_dio::flags will
also get that new flag, and iomap_dio_bio_iter() will take fs block size
into the calculation of the alignment, and pass it to
bio_iov_iter_get_pages() so that the bio will always be fs block
aligned.

For now only btrfs will utilize this flag, as btrfs needs to calculate
checksum for direct read.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/direct-io.c  | 4 ++--
 fs/iomap/direct-io.c  | 9 +++++++--
 include/linux/iomap.h | 7 +++++++
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
index 802d4dbe5b38..15aff186642d 100644
--- a/fs/btrfs/direct-io.c
+++ b/fs/btrfs/direct-io.c
@@ -763,7 +763,7 @@ static ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter,
 	struct btrfs_dio_data data = { 0 };
 
 	return iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops,
-			    IOMAP_DIO_PARTIAL, &data, done_before);
+			    IOMAP_DIO_PARTIAL | IOMAP_DIO_ALIGNED, &data, done_before);
 }
 
 static struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter,
@@ -772,7 +772,7 @@ static struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *it
 	struct btrfs_dio_data data = { 0 };
 
 	return __iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops,
-			    IOMAP_DIO_PARTIAL, &data, done_before);
+			    IOMAP_DIO_PARTIAL | IOMAP_DIO_ALIGNED, &data, done_before);
 }
 
 static ssize_t check_direct_IO(struct btrfs_fs_info *fs_info,
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 5d5d63efbd57..154bfc4ff3c4 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -336,6 +336,9 @@ 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;
+	const unsigned int alignment = (dio->flags & IOMAP_DIO_ALIGNED) ?
+		max(fs_block_size, bdev_logical_block_size(iomap->bdev)) :
+		bdev_logical_block_size(iomap->bdev);
 
 	if ((pos | length) & (bdev_logical_block_size(iomap->bdev) - 1))
 		return -EINVAL;
@@ -433,8 +436,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
 		bio->bi_private = 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);
+		ret = bio_iov_iter_get_pages(bio, dio->submit.iter, alignment - 1);
 		if (unlikely(ret)) {
 			/*
 			 * We have to stop part way through an IO. We must fall
@@ -639,6 +641,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_ALIGNED)
+		dio->flags |= IOMAP_DIO_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..9bbd36fd69cf 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -518,6 +518,13 @@ 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 data checksum for each data bio.
+ */
+#define IOMAP_DIO_ALIGNED		(1 << 3)
+
 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.50.1

Re: [PATCH] iomap: add IOMAP_DIO_ALIGNED flag for btrfs

[Christoph Hellwig]

On Mon, Oct 13, 2025 at 04:38:40PM +1030, Qu Wenruo wrote:
> For now only btrfs will utilize this flag, as btrfs needs to calculate
> checksum for direct read.

Maybe reword this as: 

The initial user of this flag is btrfs, whichs needs to calculate
the checksum for direct read and thus requires the biovec to be
file system block size aligned?

> index 802d4dbe5b38..15aff186642d 100644
> --- a/fs/btrfs/direct-io.c
> +++ b/fs/btrfs/direct-io.c

Please split the patch to use the flag in btrfs from the one adding
the the flag to iomap.

> +	const unsigned int alignment = (dio->flags & IOMAP_DIO_ALIGNED) ?
> +		max(fs_block_size, bdev_logical_block_size(iomap->bdev)) :
> +		bdev_logical_block_size(iomap->bdev);

Please unwind this into an if/else to be easily readable.  Also a
comment on why you still need the max when the flag is set would be
useful.

> +		ret = bio_iov_iter_get_pages(bio, dio->submit.iter, alignment - 1);

Please avoid overly long lines in the iomap code.

> +/*
> + * Ensure each bio is aligned to fs block size.
> + *
> + * For filesystems which need to calculate/verify data checksum for each data bio.

Another overly long line here.

> + */
> +#define IOMAP_DIO_ALIGNED		(1 << 3)

Maybe call the flag IOMAP_DIO_FSBLOCK_ALIGNED to make it clear
what alignment is implied by the flag.

Re: [PATCH] iomap: add IOMAP_DIO_ALIGNED flag for btrfs

[Qu Wenruo]

在 2025/10/13 17:03, Christoph Hellwig 写道:
> On Mon, Oct 13, 2025 at 04:38:40PM +1030, Qu Wenruo wrote:
>> For now only btrfs will utilize this flag, as btrfs needs to calculate
>> checksum for direct read.
> 
> Maybe reword this as:
> 
> The initial user of this flag is btrfs, whichs needs to calculate
> the checksum for direct read and thus requires the biovec to be
> file system block size aligned?
> 
>> index 802d4dbe5b38..15aff186642d 100644
>> --- a/fs/btrfs/direct-io.c
>> +++ b/fs/btrfs/direct-io.c
> 
> Please split the patch to use the flag in btrfs from the one adding
> the the flag to iomap.
> 
>> +	const unsigned int alignment = (dio->flags & IOMAP_DIO_ALIGNED) ?
>> +		max(fs_block_size, bdev_logical_block_size(iomap->bdev)) :
>> +		bdev_logical_block_size(iomap->bdev);
> 
> Please unwind this into an if/else to be easily readable.  Also a
> comment on why you still need the max when the flag is set would be
> useful.

The max part is mostly to be future proof, in case the bdev block size 
is larger than fs block size.
And in that case, aligning to the larger value will make everyone (block 
layer and fs layer) happy.

Will add the comment on this.

> 
>> +		ret = bio_iov_iter_get_pages(bio, dio->submit.iter, alignment - 1);
> 
> Please avoid overly long lines in the iomap code.

I'm not sure if this line (83 chars) counts as long lines.
As the recent patchchecker will only report lines over 100 chars as long.

But sure, if iomap still follows the old 80 chars limit, I'm completely 
fine to follow.

Thanks,
Qu

> 
>> +/*
>> + * Ensure each bio is aligned to fs block size.
>> + *
>> + * For filesystems which need to calculate/verify data checksum for each data bio.
> 
> Another overly long line here.
> 
>> + */
>> +#define IOMAP_DIO_ALIGNED		(1 << 3)
> 
> Maybe call the flag IOMAP_DIO_FSBLOCK_ALIGNED to make it clear
> what alignment is implied by the flag.
> 

Re: [PATCH] iomap: add IOMAP_DIO_ALIGNED flag for btrfs

[Christoph Hellwig]

On Mon, Oct 13, 2025 at 05:36:25PM +1030, Qu Wenruo wrote:
> > Please avoid overly long lines in the iomap code.
> 
> I'm not sure if this line (83 chars) counts as long lines.
> As the recent patchchecker will only report lines over 100 chars as long.

But checkpatch is as often wrong, longer than 80 is only allowed when
it improves readability.  For iomap as for many subsystems that's only
for long printk lines.