[PATCH] btrfs: get rid of btrfs_dio_private

[PATCH] btrfs: get rid of btrfs_dio_private

[Qu Wenruo]

For direct IO we need to record the file offset and size of the IO, thus
we have btrfs_dio_private structure and a dedicated bioset for it.

However for the file offset, we already btrfs_bio::file_offset, thus we
really only need a size.

Thankfully there is a hole inside btrfs_bio, between @fs_info and @bio, as
both members requires 8 bytes alignment (for 64bit systems).

The member @status and @csum_search_commit_root are both in single byte size,
leaving us 6 bytes to use.

So here we use a u32 to store the bio size, which should be more than
enough, and put the new @size member into that hole, so we can store a
new member without increasing the size of btrfs_bio.

Then we can safely remove btrfs_dio_private, the only exception is we
can not get rid of btrfs_dio_bioset, but this time we can just reuse the
existing btrfs_bio to handle everything.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/bio.c       |  4 ++++
 fs/btrfs/bio.h       |  2 ++
 fs/btrfs/direct-io.c | 34 +++++++++++-----------------------
 3 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 21df48e6c4fa..fc7182b5a8e3 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -809,7 +809,11 @@ void btrfs_submit_bbio(struct btrfs_bio *bbio, int mirror_num)
 	/* If bbio->inode is not populated, its file_offset must be 0. */
 	ASSERT(bbio->inode || bbio->file_offset == 0);
 
+	/* The bio should not exceed U32 size limit. */
+	ASSERT(bbio->bio.bi_iter.bi_size < U32_MAX);
+
 	assert_bbio_alignment(bbio);
+	bbio->size = bbio->bio.bi_iter.bi_size;
 
 	while (!btrfs_submit_chunk(bbio, mirror_num))
 		;
diff --git a/fs/btrfs/bio.h b/fs/btrfs/bio.h
index 00883aea55d7..e675937c356a 100644
--- a/fs/btrfs/bio.h
+++ b/fs/btrfs/bio.h
@@ -79,6 +79,8 @@ struct btrfs_bio {
 	/* File system that this I/O operates on. */
 	struct btrfs_fs_info *fs_info;
 
+	u32 size;
+
 	/* Save the first error status of split bio. */
 	blk_status_t status;
 
diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
index 802d4dbe5b38..4dd901dc4abc 100644
--- a/fs/btrfs/direct-io.c
+++ b/fs/btrfs/direct-io.c
@@ -19,15 +19,6 @@ struct btrfs_dio_data {
 	bool nocow_done;
 };
 
-struct btrfs_dio_private {
-	/* Range of I/O */
-	u64 file_offset;
-	u32 bytes;
-
-	/* This must be last */
-	struct btrfs_bio bbio;
-};
-
 static struct bio_set btrfs_dio_bioset;
 
 static int lock_extent_direct(struct inode *inode, u64 lockstart, u64 lockend,
@@ -642,25 +633,26 @@ static int btrfs_dio_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 
 static void btrfs_dio_end_io(struct btrfs_bio *bbio)
 {
-	struct btrfs_dio_private *dip =
-		container_of(bbio, struct btrfs_dio_private, bbio);
 	struct btrfs_inode *inode = bbio->inode;
 	struct bio *bio = &bbio->bio;
 
+	/* We should have bbio->bytes populated. */
+	ASSERT(bbio->size);
+
 	if (bio->bi_status) {
 		btrfs_warn(inode->root->fs_info,
 		"direct IO failed ino %llu op 0x%0x offset %#llx len %u err no %d",
 			   btrfs_ino(inode), bio->bi_opf,
-			   dip->file_offset, dip->bytes, bio->bi_status);
+			   bbio->file_offset, bbio->size, bio->bi_status);
 	}
 
 	if (btrfs_op(bio) == BTRFS_MAP_WRITE) {
 		btrfs_finish_ordered_extent(bbio->ordered, NULL,
-					    dip->file_offset, dip->bytes,
+					    bbio->file_offset, bbio->size,
 					    !bio->bi_status);
 	} else {
-		btrfs_unlock_dio_extent(&inode->io_tree, dip->file_offset,
-					dip->file_offset + dip->bytes - 1, NULL);
+		btrfs_unlock_dio_extent(&inode->io_tree, bbio->file_offset,
+					bbio->file_offset + bbio->size - 1, NULL);
 	}
 
 	bbio->bio.bi_private = bbio->private;
@@ -709,19 +701,15 @@ static void btrfs_dio_submit_io(const struct iomap_iter *iter, struct bio *bio,
 				loff_t file_offset)
 {
 	struct btrfs_bio *bbio = btrfs_bio(bio);
-	struct btrfs_dio_private *dip =
-		container_of(bbio, struct btrfs_dio_private, bbio);
 	struct btrfs_dio_data *dio_data = iter->private;
+	const u32 bio_size = bio->bi_iter.bi_size;
 
 	btrfs_bio_init(bbio, BTRFS_I(iter->inode)->root->fs_info,
 		       btrfs_dio_end_io, bio->bi_private);
 	bbio->inode = BTRFS_I(iter->inode);
 	bbio->file_offset = file_offset;
 
-	dip->file_offset = file_offset;
-	dip->bytes = bio->bi_iter.bi_size;
-
-	dio_data->submitted += bio->bi_iter.bi_size;
+	dio_data->submitted += bio_size;
 
 	/*
 	 * Check if we are doing a partial write.  If we are, we need to split
@@ -736,7 +724,7 @@ static void btrfs_dio_submit_io(const struct iomap_iter *iter, struct bio *bio,
 		ret = btrfs_extract_ordered_extent(bbio, dio_data->ordered);
 		if (ret) {
 			btrfs_finish_ordered_extent(dio_data->ordered, NULL,
-						    file_offset, dip->bytes,
+						    file_offset, bio_size,
 						    !ret);
 			bio->bi_status = errno_to_blk_status(ret);
 			iomap_dio_bio_end_io(bio);
@@ -1093,7 +1081,7 @@ ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to)
 int __init btrfs_init_dio(void)
 {
 	if (bioset_init(&btrfs_dio_bioset, BIO_POOL_SIZE,
-			offsetof(struct btrfs_dio_private, bbio.bio),
+			offsetof(struct btrfs_bio, bio),
 			BIOSET_NEED_BVECS))
 		return -ENOMEM;
 
-- 
2.51.0

Re: [PATCH] btrfs: get rid of btrfs_dio_private

[Johannes Thumshirn]

     /* size: 336, cachelines: 6, members: 13 */
     /* sum members: 334, holes: 1, sum holes: 2 */
     /* member types with holes: 1, total: 1 */
     /* forced alignments: 2, forced holes: 1, sum forced holes: 2 */
     /* last cacheline: 16 bytes */


We might want to shuffle some things around though.

Anyways, not relevant to this patch

Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH] btrfs: get rid of btrfs_dio_private

[Qu Wenruo]

在 2025/10/20 17:41, Johannes Thumshirn 写道:
>       /* size: 336, cachelines: 6, members: 13 */
>       /* sum members: 334, holes: 1, sum holes: 2 */
>       /* member types with holes: 1, total: 1 */
>       /* forced alignments: 2, forced holes: 1, sum forced holes: 2 */
>       /* last cacheline: 16 bytes */
> 
> 
> We might want to shuffle some things around though.
> 
> Anyways, not relevant to this patch

Unfortunately fstests is not passing, test cases like btrfs/06[01] can 
fail with pretty high chance.

It looks like the bbio->file_offset is not safe, as btrfs_split_bio() 
can modify the original file offset, causing some bio ranges not 
properly released during dio reads.

Please ignore the patchset for now, we really want some stable 
file_offset/size that will not change due to split.

Thanks,
Qu

> 
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> 
> 

Re: [PATCH] btrfs: get rid of btrfs_dio_private

[Christoph Hellwig]

On Mon, Oct 20, 2025 at 05:46:23PM +1030, Qu Wenruo wrote:
> 
> It looks like the bbio->file_offset is not safe, as btrfs_split_bio() can
> modify the original file offset, causing some bio ranges not properly
> released during dio reads.
> 
> Please ignore the patchset for now, we really want some stable
> file_offset/size that will not change due to split.

You can't make them stable because they must change for splits.

You could do what XFS/iomapo do: split the ordered extents on bio
boundaries, and then in the completion handler merge them again
for more efficient handling if there are enough queued up.