All of lore.kernel.org
 help / color / mirror / Atom feed
* extent_io bio submission cleanup
@ 2022-06-03  7:11 Christoph Hellwig
  2022-06-03  7:11 ` [PATCH 1/3] btrfs: don't use bio->bi_private to pass the inode to submit_one_bio Christoph Hellwig
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Christoph Hellwig @ 2022-06-03  7:11 UTC (permalink / raw)
  To: Josef Bacik, David Sterba; +Cc: linux-btrfs

Hi all,

this series cleans up how extent_io.c submits bios.

Diffstat:
 extent_io.c |  168 +++++++++++++++++++++---------------------------------------
 1 file changed, 61 insertions(+), 107 deletions(-)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] btrfs: don't use bio->bi_private to pass the inode to submit_one_bio
  2022-06-03  7:11 extent_io bio submission cleanup Christoph Hellwig
@ 2022-06-03  7:11 ` Christoph Hellwig
  2022-06-03 10:33   ` Qu Wenruo
  2022-06-03  7:11 ` [PATCH 2/3] btrfs: merge end_write_bio and flush_write_bio Christoph Hellwig
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2022-06-03  7:11 UTC (permalink / raw)
  To: Josef Bacik, David Sterba; +Cc: linux-btrfs

submit_one_bio is only used for page cache I/O, so the inode can be
trivially derived from the first page in the bio.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/extent_io.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 7b5f872d2eb9f..025349aeec31f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -181,10 +181,7 @@ static int add_extent_changeset(struct extent_state *state, u32 bits,
 static void submit_one_bio(struct bio *bio, int mirror_num,
 			   enum btrfs_compression_type compress_type)
 {
-	struct extent_io_tree *tree = bio->bi_private;
-	struct inode *inode = tree->private_data;
-
-	bio->bi_private = NULL;
+	struct inode *inode = bio_first_page_all(bio)->mapping->host;
 
 	/* Caller should ensure the bio has at least some range added */
 	ASSERT(bio->bi_iter.bi_size);
@@ -3360,7 +3357,6 @@ static int alloc_new_bio(struct btrfs_inode *inode,
 	bio_ctrl->bio = bio;
 	bio_ctrl->compress_type = compress_type;
 	bio->bi_end_io = end_io_func;
-	bio->bi_private = &inode->io_tree;
 	bio->bi_opf = opf;
 	ret = calc_bio_boundaries(bio_ctrl, inode, file_offset);
 	if (ret < 0)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] btrfs: merge end_write_bio and flush_write_bio
  2022-06-03  7:11 extent_io bio submission cleanup Christoph Hellwig
  2022-06-03  7:11 ` [PATCH 1/3] btrfs: don't use bio->bi_private to pass the inode to submit_one_bio Christoph Hellwig
@ 2022-06-03  7:11 ` Christoph Hellwig
  2022-06-03 10:40   ` Qu Wenruo
  2022-06-03  7:11 ` [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio Christoph Hellwig
  2022-06-06 20:26 ` extent_io bio submission cleanup David Sterba
  3 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2022-06-03  7:11 UTC (permalink / raw)
  To: Josef Bacik, David Sterba; +Cc: linux-btrfs

Merge end_write_bio and flush_write_bio into a single submit_write_bio
helper, that either submits the bio or ends it if a negative errno was
passed in.  This consolidates a lot of duplicated checks in the callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/extent_io.c | 94 ++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 64 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 025349aeec31f..72a258fa27947 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -201,39 +201,26 @@ static void submit_one_bio(struct bio *bio, int mirror_num,
 	 */
 }
 
-/* Cleanup unsubmitted bios */
-static void end_write_bio(struct extent_page_data *epd, int ret)
-{
-	struct bio *bio = epd->bio_ctrl.bio;
-
-	if (bio) {
-		bio->bi_status = errno_to_blk_status(ret);
-		bio_endio(bio);
-		epd->bio_ctrl.bio = NULL;
-	}
-}
-
 /*
- * Submit bio from extent page data via submit_one_bio
- *
- * Return 0 if everything is OK.
- * Return <0 for error.
+ * Submit or fail the current bio in an extent_page_data structure.
  */
-static void flush_write_bio(struct extent_page_data *epd)
+static void submit_write_bio(struct extent_page_data *epd, int ret)
 {
 	struct bio *bio = epd->bio_ctrl.bio;
 
-	if (bio) {
+	if (!bio)
+		return;
+
+	if (ret) {
+		ASSERT(ret < 0);
+		bio->bi_status = errno_to_blk_status(ret);
+		bio_endio(bio);
+	} else {
 		submit_one_bio(bio, 0, 0);
-		/*
-		 * Clean up of epd->bio is handled by its endio function.
-		 * And endio is either triggered by successful bio execution
-		 * or the error handler of submit bio hook.
-		 * So at this point, no matter what happened, we don't need
-		 * to clean up epd->bio.
-		 */
-		epd->bio_ctrl.bio = NULL;
 	}
+
+	/* The bio is owned by the bi_end_io handler now */
+	epd->bio_ctrl.bio = NULL;
 }
 
 int __init extent_state_cache_init(void)
@@ -4248,7 +4235,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
 	int ret = 0;
 
 	if (!btrfs_try_tree_write_lock(eb)) {
-		flush_write_bio(epd);
+		submit_write_bio(epd, 0);
 		flush = 1;
 		btrfs_tree_lock(eb);
 	}
@@ -4258,7 +4245,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
 		if (!epd->sync_io)
 			return 0;
 		if (!flush) {
-			flush_write_bio(epd);
+			submit_write_bio(epd, 0);
 			flush = 1;
 		}
 		while (1) {
@@ -4305,7 +4292,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
 
 		if (!trylock_page(p)) {
 			if (!flush) {
-				flush_write_bio(epd);
+				submit_write_bio(epd, 0);
 				flush = 1;
 			}
 			lock_page(p);
@@ -4721,7 +4708,7 @@ static int submit_eb_subpage(struct page *page,
 
 cleanup:
 	/* We hit error, end bio for the submitted extent buffers */
-	end_write_bio(epd, ret);
+	submit_write_bio(epd, ret);
 	return ret;
 }
 
@@ -4900,10 +4887,6 @@ int btree_write_cache_pages(struct address_space *mapping,
 		index = 0;
 		goto retry;
 	}
-	if (ret < 0) {
-		end_write_bio(&epd, ret);
-		goto out;
-	}
 	/*
 	 * If something went wrong, don't allow any metadata write bio to be
 	 * submitted.
@@ -4930,21 +4913,17 @@ int btree_write_cache_pages(struct address_space *mapping,
 	 *   Now such dirty tree block will not be cleaned by any dirty
 	 *   extent io tree. Thus we don't want to submit such wild eb
 	 *   if the fs already has error.
-	 */
-	if (!BTRFS_FS_ERROR(fs_info)) {
-		flush_write_bio(&epd);
-	} else {
-		ret = -EROFS;
-		end_write_bio(&epd, ret);
-	}
-out:
-	btrfs_zoned_meta_io_unlock(fs_info);
-	/*
+	 *
 	 * We can get ret > 0 from submit_extent_page() indicating how many ebs
 	 * were submitted. Reset it to 0 to avoid false alerts for the caller.
 	 */
 	if (ret > 0)
 		ret = 0;
+	if (!ret && BTRFS_FS_ERROR(fs_info))
+		ret = -EROFS;
+	submit_write_bio(&epd, ret);
+
+	btrfs_zoned_meta_io_unlock(fs_info);
 	return ret;
 }
 
@@ -5046,7 +5025,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
 			 * tmpfs file mapping
 			 */
 			if (!trylock_page(page)) {
-				flush_write_bio(epd);
+				submit_write_bio(epd, 0);
 				lock_page(page);
 			}
 
@@ -5057,7 +5036,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
 
 			if (wbc->sync_mode != WB_SYNC_NONE) {
 				if (PageWriteback(page))
-					flush_write_bio(epd);
+					submit_write_bio(epd, 0);
 				wait_on_page_writeback(page);
 			}
 
@@ -5097,7 +5076,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
 		 * page in our current bio, and thus deadlock, so flush the
 		 * write bio here.
 		 */
-		flush_write_bio(epd);
+		submit_write_bio(epd, 0);
 		goto retry;
 	}
 
@@ -5118,13 +5097,7 @@ int extent_write_full_page(struct page *page, struct writeback_control *wbc)
 	};
 
 	ret = __extent_writepage(page, wbc, &epd);
-	ASSERT(ret <= 0);
-	if (ret < 0) {
-		end_write_bio(&epd, ret);
-		return ret;
-	}
-
-	flush_write_bio(&epd);
+	submit_write_bio(&epd, ret);
 	return ret;
 }
 
@@ -5185,10 +5158,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
 		cur = cur_end + 1;
 	}
 
-	if (!found_error)
-		flush_write_bio(&epd);
-	else
-		end_write_bio(&epd, ret);
+	submit_write_bio(&epd, found_error ? ret : 0);
 
 	wbc_detach_inode(&wbc_writepages);
 	if (found_error)
@@ -5214,12 +5184,8 @@ int extent_writepages(struct address_space *mapping,
 	btrfs_zoned_data_reloc_lock(BTRFS_I(inode));
 	ret = extent_write_cache_pages(mapping, wbc, &epd);
 	btrfs_zoned_data_reloc_unlock(BTRFS_I(inode));
-	ASSERT(ret <= 0);
-	if (ret < 0) {
-		end_write_bio(&epd, ret);
-		return ret;
-	}
-	flush_write_bio(&epd);
+
+	submit_write_bio(&epd, ret);
 	return ret;
 }
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio
  2022-06-03  7:11 extent_io bio submission cleanup Christoph Hellwig
  2022-06-03  7:11 ` [PATCH 1/3] btrfs: don't use bio->bi_private to pass the inode to submit_one_bio Christoph Hellwig
  2022-06-03  7:11 ` [PATCH 2/3] btrfs: merge end_write_bio and flush_write_bio Christoph Hellwig
@ 2022-06-03  7:11 ` Christoph Hellwig
  2022-06-04 22:31   ` Qu Wenruo
  2022-06-06 10:41   ` Nikolay Borisov
  2022-06-06 20:26 ` extent_io bio submission cleanup David Sterba
  3 siblings, 2 replies; 12+ messages in thread
From: Christoph Hellwig @ 2022-06-03  7:11 UTC (permalink / raw)
  To: Josef Bacik, David Sterba; +Cc: linux-btrfs

submit_one_bio always works on the bio and compression flags from a
btrfs_bio_ctrl structure.  Pass the explicitly and clean up the the
calling conventions by handling a NULL bio in submit_one_bio, and
using the btrfs_bio_ctrl to pass the mirror number as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/extent_io.c | 82 ++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 45 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 72a258fa27947..facca7feb9a22 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -144,6 +144,7 @@ struct tree_entry {
  */
 struct btrfs_bio_ctrl {
 	struct bio *bio;
+	int mirror_num;
 	enum btrfs_compression_type compress_type;
 	u32 len_to_stripe_boundary;
 	u32 len_to_oe_boundary;
@@ -178,10 +179,11 @@ static int add_extent_changeset(struct extent_state *state, u32 bits,
 	return ret;
 }
 
-static void submit_one_bio(struct bio *bio, int mirror_num,
-			   enum btrfs_compression_type compress_type)
+static void __submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
 {
+	struct bio *bio = bio_ctrl->bio;
 	struct inode *inode = bio_first_page_all(bio)->mapping->host;
+	int mirror_num = bio_ctrl->mirror_num;
 
 	/* Caller should ensure the bio has at least some range added */
 	ASSERT(bio->bi_iter.bi_size);
@@ -191,14 +193,17 @@ static void submit_one_bio(struct bio *bio, int mirror_num,
 	else if (btrfs_op(bio) == BTRFS_MAP_WRITE)
 		btrfs_submit_data_write_bio(inode, bio, mirror_num);
 	else
-		btrfs_submit_data_read_bio(inode, bio, mirror_num, compress_type);
+		btrfs_submit_data_read_bio(inode, bio, mirror_num,
+					   bio_ctrl->compress_type);
 
-	/*
-	 * Above submission hooks will handle the error by ending the bio,
-	 * which will do the cleanup properly.  So here we should not return
-	 * any error, or the caller of submit_extent_page() will do cleanup
-	 * again, causing problems.
-	 */
+	/* The bio is owned by the bi_end_io handler now */
+	bio_ctrl->bio = NULL;
+}
+
+static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
+{
+	if (bio_ctrl->bio)
+		__submit_one_bio(bio_ctrl);
 }
 
 /*
@@ -215,12 +220,11 @@ static void submit_write_bio(struct extent_page_data *epd, int ret)
 		ASSERT(ret < 0);
 		bio->bi_status = errno_to_blk_status(ret);
 		bio_endio(bio);
+		/* The bio is owned by the bi_end_io handler now */
+		epd->bio_ctrl.bio = NULL;
 	} else {
-		submit_one_bio(bio, 0, 0);
+		__submit_one_bio(&epd->bio_ctrl);
 	}
-
-	/* The bio is owned by the bi_end_io handler now */
-	epd->bio_ctrl.bio = NULL;
 }
 
 int __init extent_state_cache_init(void)
@@ -3408,7 +3412,6 @@ static int submit_extent_page(unsigned int opf,
 			      struct page *page, u64 disk_bytenr,
 			      size_t size, unsigned long pg_offset,
 			      bio_end_io_t end_io_func,
-			      int mirror_num,
 			      enum btrfs_compression_type compress_type,
 			      bool force_bio_submit)
 {
@@ -3420,10 +3423,8 @@ static int submit_extent_page(unsigned int opf,
 
 	ASSERT(pg_offset < PAGE_SIZE && size <= PAGE_SIZE &&
 	       pg_offset + size <= PAGE_SIZE);
-	if (force_bio_submit && bio_ctrl->bio) {
-		submit_one_bio(bio_ctrl->bio, mirror_num, bio_ctrl->compress_type);
-		bio_ctrl->bio = NULL;
-	}
+	if (force_bio_submit)
+		submit_one_bio(bio_ctrl);
 
 	while (cur < pg_offset + size) {
 		u32 offset = cur - pg_offset;
@@ -3463,8 +3464,7 @@ static int submit_extent_page(unsigned int opf,
 		if (added < size - offset) {
 			/* The bio should contain some page(s) */
 			ASSERT(bio_ctrl->bio->bi_iter.bi_size);
-			submit_one_bio(bio_ctrl->bio, mirror_num, bio_ctrl->compress_type);
-			bio_ctrl->bio = NULL;
+			submit_one_bio(bio_ctrl);
 		}
 		cur += added;
 	}
@@ -3741,10 +3741,8 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 
 		ret = submit_extent_page(REQ_OP_READ | read_flags, NULL,
 					 bio_ctrl, page, disk_bytenr, iosize,
-					 pg_offset,
-					 end_bio_extent_readpage, 0,
-					 this_bio_flag,
-					 force_bio_submit);
+					 pg_offset, end_bio_extent_readpage,
+					 this_bio_flag, force_bio_submit);
 		if (ret) {
 			/*
 			 * We have to unlock the remaining range, or the page
@@ -3776,8 +3774,7 @@ int btrfs_readpage(struct file *file, struct page *page)
 	 * If btrfs_do_readpage() failed we will want to submit the assembled
 	 * bio to do the cleanup.
 	 */
-	if (bio_ctrl.bio)
-		submit_one_bio(bio_ctrl.bio, 0, bio_ctrl.compress_type);
+	submit_one_bio(&bio_ctrl);
 	return ret;
 }
 
@@ -4060,7 +4057,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
 					 disk_bytenr, iosize,
 					 cur - page_offset(page),
 					 end_bio_extent_writepage,
-					 0, 0, false);
+					 0, false);
 		if (ret) {
 			has_error = true;
 			if (!saved_ret)
@@ -4553,7 +4550,7 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
 	ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc,
 			&epd->bio_ctrl, page, eb->start, eb->len,
 			eb->start - page_offset(page),
-			end_bio_subpage_eb_writepage, 0, 0, false);
+			end_bio_subpage_eb_writepage, 0, false);
 	if (ret) {
 		btrfs_subpage_clear_writeback(fs_info, page, eb->start, eb->len);
 		set_btree_ioerr(page, eb);
@@ -4594,7 +4591,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
 					 &epd->bio_ctrl, p, disk_bytenr,
 					 PAGE_SIZE, 0,
 					 end_bio_extent_buffer_writepage,
-					 0, 0, false);
+					 0, false);
 		if (ret) {
 			set_btree_ioerr(p, eb);
 			if (PageWriteback(p))
@@ -5207,9 +5204,7 @@ void extent_readahead(struct readahead_control *rac)
 
 	if (em_cached)
 		free_extent_map(em_cached);
-
-	if (bio_ctrl.bio)
-		submit_one_bio(bio_ctrl.bio, 0, bio_ctrl.compress_type);
+	submit_one_bio(&bio_ctrl);
 }
 
 /*
@@ -6536,7 +6531,9 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
 	struct btrfs_fs_info *fs_info = eb->fs_info;
 	struct extent_io_tree *io_tree;
 	struct page *page = eb->pages[0];
-	struct btrfs_bio_ctrl bio_ctrl = { 0 };
+	struct btrfs_bio_ctrl bio_ctrl = {
+		.mirror_num = mirror_num,
+	};
 	int ret = 0;
 
 	ASSERT(!test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags));
@@ -6571,8 +6568,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
 	ret = submit_extent_page(REQ_OP_READ, NULL, &bio_ctrl,
 				 page, eb->start, eb->len,
 				 eb->start - page_offset(page),
-				 end_bio_extent_readpage, mirror_num, 0,
-				 true);
+				 end_bio_extent_readpage, 0, true);
 	if (ret) {
 		/*
 		 * In the endio function, if we hit something wrong we will
@@ -6581,10 +6577,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
 		 */
 		atomic_dec(&eb->io_pages);
 	}
-	if (bio_ctrl.bio) {
-		submit_one_bio(bio_ctrl.bio, mirror_num, 0);
-		bio_ctrl.bio = NULL;
-	}
+	submit_one_bio(&bio_ctrl);
 	if (ret || wait != WAIT_COMPLETE)
 		return ret;
 
@@ -6604,7 +6597,9 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
 	int all_uptodate = 1;
 	int num_pages;
 	unsigned long num_reads = 0;
-	struct btrfs_bio_ctrl bio_ctrl = { 0 };
+	struct btrfs_bio_ctrl bio_ctrl = {
+		.mirror_num = mirror_num,
+	};
 
 	if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
 		return 0;
@@ -6678,7 +6673,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
 			err = submit_extent_page(REQ_OP_READ, NULL,
 					 &bio_ctrl, page, page_offset(page),
 					 PAGE_SIZE, 0, end_bio_extent_readpage,
-					 mirror_num, 0, false);
+					 0, false);
 			if (err) {
 				/*
 				 * We failed to submit the bio so it's the
@@ -6695,10 +6690,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
 		}
 	}
 
-	if (bio_ctrl.bio) {
-		submit_one_bio(bio_ctrl.bio, mirror_num, bio_ctrl.compress_type);
-		bio_ctrl.bio = NULL;
-	}
+	submit_one_bio(&bio_ctrl);
 
 	if (ret || wait != WAIT_COMPLETE)
 		return ret;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] btrfs: don't use bio->bi_private to pass the inode to submit_one_bio
  2022-06-03  7:11 ` [PATCH 1/3] btrfs: don't use bio->bi_private to pass the inode to submit_one_bio Christoph Hellwig
@ 2022-06-03 10:33   ` Qu Wenruo
  0 siblings, 0 replies; 12+ messages in thread
From: Qu Wenruo @ 2022-06-03 10:33 UTC (permalink / raw)
  To: Christoph Hellwig, Josef Bacik, David Sterba; +Cc: linux-btrfs



On 2022/6/3 15:11, Christoph Hellwig wrote:
> submit_one_bio is only used for page cache I/O, so the inode can be
> trivially derived from the first page in the bio.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Qu Wenruo <wqu@suse.com>

> ---
>   fs/btrfs/extent_io.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 7b5f872d2eb9f..025349aeec31f 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -181,10 +181,7 @@ static int add_extent_changeset(struct extent_state *state, u32 bits,
>   static void submit_one_bio(struct bio *bio, int mirror_num,
>   			   enum btrfs_compression_type compress_type)
>   {
> -	struct extent_io_tree *tree = bio->bi_private;
> -	struct inode *inode = tree->private_data;
> -
> -	bio->bi_private = NULL;
> +	struct inode *inode = bio_first_page_all(bio)->mapping->host;
>
>   	/* Caller should ensure the bio has at least some range added */
>   	ASSERT(bio->bi_iter.bi_size);
> @@ -3360,7 +3357,6 @@ static int alloc_new_bio(struct btrfs_inode *inode,
>   	bio_ctrl->bio = bio;
>   	bio_ctrl->compress_type = compress_type;
>   	bio->bi_end_io = end_io_func;
> -	bio->bi_private = &inode->io_tree;

And what the heck that we're passing io_tree into bi_private??
At least we should try passing inode.

Anyway, the old behavior makes no sense, great we can get rid of it.

BTW, I didn't see modification in btrfs_repair_one_sector(), it is not a
problem since we no longer utilize bi_private, but it may be better
explicitly remove that call site too.

Thanks,
Qu
>   	bio->bi_opf = opf;
>   	ret = calc_bio_boundaries(bio_ctrl, inode, file_offset);
>   	if (ret < 0)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] btrfs: merge end_write_bio and flush_write_bio
  2022-06-03  7:11 ` [PATCH 2/3] btrfs: merge end_write_bio and flush_write_bio Christoph Hellwig
@ 2022-06-03 10:40   ` Qu Wenruo
  0 siblings, 0 replies; 12+ messages in thread
From: Qu Wenruo @ 2022-06-03 10:40 UTC (permalink / raw)
  To: Christoph Hellwig, Josef Bacik, David Sterba; +Cc: linux-btrfs



On 2022/6/3 15:11, Christoph Hellwig wrote:
> Merge end_write_bio and flush_write_bio into a single submit_write_bio
> helper, that either submits the bio or ends it if a negative errno was
> passed in.  This consolidates a lot of duplicated checks in the callers.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/extent_io.c | 94 ++++++++++++++------------------------------
>   1 file changed, 30 insertions(+), 64 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 025349aeec31f..72a258fa27947 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -201,39 +201,26 @@ static void submit_one_bio(struct bio *bio, int mirror_num,
>   	 */
>   }
>
> -/* Cleanup unsubmitted bios */
> -static void end_write_bio(struct extent_page_data *epd, int ret)
> -{
> -	struct bio *bio = epd->bio_ctrl.bio;
> -
> -	if (bio) {
> -		bio->bi_status = errno_to_blk_status(ret);
> -		bio_endio(bio);
> -		epd->bio_ctrl.bio = NULL;
> -	}
> -}
> -
>   /*
> - * Submit bio from extent page data via submit_one_bio
> - *
> - * Return 0 if everything is OK.
> - * Return <0 for error.
> + * Submit or fail the current bio in an extent_page_data structure.
>    */
> -static void flush_write_bio(struct extent_page_data *epd)
> +static void submit_write_bio(struct extent_page_data *epd, int ret)
>   {
>   	struct bio *bio = epd->bio_ctrl.bio;
>
> -	if (bio) {
> +	if (!bio)
> +		return;
> +
> +	if (ret) {
> +		ASSERT(ret < 0);
> +		bio->bi_status = errno_to_blk_status(ret);
> +		bio_endio(bio);
> +	} else {
>   		submit_one_bio(bio, 0, 0);
> -		/*
> -		 * Clean up of epd->bio is handled by its endio function.
> -		 * And endio is either triggered by successful bio execution
> -		 * or the error handler of submit bio hook.
> -		 * So at this point, no matter what happened, we don't need
> -		 * to clean up epd->bio.
> -		 */
> -		epd->bio_ctrl.bio = NULL;
>   	}
> +
> +	/* The bio is owned by the bi_end_io handler now */
> +	epd->bio_ctrl.bio = NULL;
>   }
>
>   int __init extent_state_cache_init(void)
> @@ -4248,7 +4235,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
>   	int ret = 0;
>
>   	if (!btrfs_try_tree_write_lock(eb)) {
> -		flush_write_bio(epd);
> +		submit_write_bio(epd, 0);
>   		flush = 1;
>   		btrfs_tree_lock(eb);
>   	}
> @@ -4258,7 +4245,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
>   		if (!epd->sync_io)
>   			return 0;
>   		if (!flush) {
> -			flush_write_bio(epd);
> +			submit_write_bio(epd, 0);
>   			flush = 1;
>   		}
>   		while (1) {
> @@ -4305,7 +4292,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
>
>   		if (!trylock_page(p)) {
>   			if (!flush) {
> -				flush_write_bio(epd);
> +				submit_write_bio(epd, 0);
>   				flush = 1;
>   			}
>   			lock_page(p);
> @@ -4721,7 +4708,7 @@ static int submit_eb_subpage(struct page *page,
>
>   cleanup:
>   	/* We hit error, end bio for the submitted extent buffers */
> -	end_write_bio(epd, ret);
> +	submit_write_bio(epd, ret);
>   	return ret;
>   }
>
> @@ -4900,10 +4887,6 @@ int btree_write_cache_pages(struct address_space *mapping,
>   		index = 0;
>   		goto retry;
>   	}
> -	if (ret < 0) {
> -		end_write_bio(&epd, ret);
> -		goto out;
> -	}
>   	/*
>   	 * If something went wrong, don't allow any metadata write bio to be
>   	 * submitted.
> @@ -4930,21 +4913,17 @@ int btree_write_cache_pages(struct address_space *mapping,
>   	 *   Now such dirty tree block will not be cleaned by any dirty
>   	 *   extent io tree. Thus we don't want to submit such wild eb
>   	 *   if the fs already has error.
> -	 */
> -	if (!BTRFS_FS_ERROR(fs_info)) {
> -		flush_write_bio(&epd);
> -	} else {
> -		ret = -EROFS;
> -		end_write_bio(&epd, ret);
> -	}
> -out:
> -	btrfs_zoned_meta_io_unlock(fs_info);
> -	/*
> +	 *
>   	 * We can get ret > 0 from submit_extent_page() indicating how many ebs
>   	 * were submitted. Reset it to 0 to avoid false alerts for the caller.
>   	 */
>   	if (ret > 0)
>   		ret = 0;
> +	if (!ret && BTRFS_FS_ERROR(fs_info))
> +		ret = -EROFS;
> +	submit_write_bio(&epd, ret);
> +
> +	btrfs_zoned_meta_io_unlock(fs_info);
>   	return ret;
>   }
>
> @@ -5046,7 +5025,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
>   			 * tmpfs file mapping
>   			 */
>   			if (!trylock_page(page)) {
> -				flush_write_bio(epd);
> +				submit_write_bio(epd, 0);
>   				lock_page(page);
>   			}
>
> @@ -5057,7 +5036,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
>
>   			if (wbc->sync_mode != WB_SYNC_NONE) {
>   				if (PageWriteback(page))
> -					flush_write_bio(epd);
> +					submit_write_bio(epd, 0);
>   				wait_on_page_writeback(page);
>   			}
>
> @@ -5097,7 +5076,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
>   		 * page in our current bio, and thus deadlock, so flush the
>   		 * write bio here.
>   		 */
> -		flush_write_bio(epd);
> +		submit_write_bio(epd, 0);
>   		goto retry;
>   	}
>
> @@ -5118,13 +5097,7 @@ int extent_write_full_page(struct page *page, struct writeback_control *wbc)
>   	};
>
>   	ret = __extent_writepage(page, wbc, &epd);
> -	ASSERT(ret <= 0);
> -	if (ret < 0) {
> -		end_write_bio(&epd, ret);
> -		return ret;
> -	}
> -
> -	flush_write_bio(&epd);
> +	submit_write_bio(&epd, ret);
>   	return ret;
>   }
>
> @@ -5185,10 +5158,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
>   		cur = cur_end + 1;
>   	}
>
> -	if (!found_error)
> -		flush_write_bio(&epd);
> -	else
> -		end_write_bio(&epd, ret);
> +	submit_write_bio(&epd, found_error ? ret : 0);
>
>   	wbc_detach_inode(&wbc_writepages);
>   	if (found_error)
> @@ -5214,12 +5184,8 @@ int extent_writepages(struct address_space *mapping,
>   	btrfs_zoned_data_reloc_lock(BTRFS_I(inode));
>   	ret = extent_write_cache_pages(mapping, wbc, &epd);
>   	btrfs_zoned_data_reloc_unlock(BTRFS_I(inode));
> -	ASSERT(ret <= 0);
> -	if (ret < 0) {
> -		end_write_bio(&epd, ret);
> -		return ret;
> -	}
> -	flush_write_bio(&epd);
> +
> +	submit_write_bio(&epd, ret);
>   	return ret;
>   }
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio
  2022-06-03  7:11 ` [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio Christoph Hellwig
@ 2022-06-04 22:31   ` Qu Wenruo
  2022-06-06  6:30     ` Christoph Hellwig
  2022-06-06 10:41   ` Nikolay Borisov
  1 sibling, 1 reply; 12+ messages in thread
From: Qu Wenruo @ 2022-06-04 22:31 UTC (permalink / raw)
  To: Christoph Hellwig, Josef Bacik, David Sterba; +Cc: linux-btrfs



On 2022/6/3 15:11, Christoph Hellwig wrote:
> submit_one_bio always works on the bio and compression flags from a
> btrfs_bio_ctrl structure.  Pass the explicitly and clean up the the
> calling conventions by handling a NULL bio in submit_one_bio, and
> using the btrfs_bio_ctrl to pass the mirror number as well.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Qu Wenruo <wqu@suse.com>

In fact the only call sites really caring num_mirror is the metadata
read path (as it doesn't rely on the read-repair code, since metadata
has inline csum and it has a different validation condition).

It may be a good idea to make a union for btrfs_bio, to contain all the
needed info for metadata verification (btrfs_key, transid, level), so
that we can get rid of the num_mirror parameter for submit_extent_page()
completely.

Thanks,
Qu
> ---
>   fs/btrfs/extent_io.c | 82 ++++++++++++++++++++------------------------
>   1 file changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 72a258fa27947..facca7feb9a22 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -144,6 +144,7 @@ struct tree_entry {
>    */
>   struct btrfs_bio_ctrl {
>   	struct bio *bio;
> +	int mirror_num;
>   	enum btrfs_compression_type compress_type;
>   	u32 len_to_stripe_boundary;
>   	u32 len_to_oe_boundary;
> @@ -178,10 +179,11 @@ static int add_extent_changeset(struct extent_state *state, u32 bits,
>   	return ret;
>   }
>
> -static void submit_one_bio(struct bio *bio, int mirror_num,
> -			   enum btrfs_compression_type compress_type)
> +static void __submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
>   {
> +	struct bio *bio = bio_ctrl->bio;
>   	struct inode *inode = bio_first_page_all(bio)->mapping->host;
> +	int mirror_num = bio_ctrl->mirror_num;
>
>   	/* Caller should ensure the bio has at least some range added */
>   	ASSERT(bio->bi_iter.bi_size);
> @@ -191,14 +193,17 @@ static void submit_one_bio(struct bio *bio, int mirror_num,
>   	else if (btrfs_op(bio) == BTRFS_MAP_WRITE)
>   		btrfs_submit_data_write_bio(inode, bio, mirror_num);
>   	else
> -		btrfs_submit_data_read_bio(inode, bio, mirror_num, compress_type);
> +		btrfs_submit_data_read_bio(inode, bio, mirror_num,
> +					   bio_ctrl->compress_type);
>
> -	/*
> -	 * Above submission hooks will handle the error by ending the bio,
> -	 * which will do the cleanup properly.  So here we should not return
> -	 * any error, or the caller of submit_extent_page() will do cleanup
> -	 * again, causing problems.
> -	 */
> +	/* The bio is owned by the bi_end_io handler now */
> +	bio_ctrl->bio = NULL;
> +}
> +
> +static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
> +{
> +	if (bio_ctrl->bio)
> +		__submit_one_bio(bio_ctrl);
>   }
>
>   /*
> @@ -215,12 +220,11 @@ static void submit_write_bio(struct extent_page_data *epd, int ret)
>   		ASSERT(ret < 0);
>   		bio->bi_status = errno_to_blk_status(ret);
>   		bio_endio(bio);
> +		/* The bio is owned by the bi_end_io handler now */
> +		epd->bio_ctrl.bio = NULL;
>   	} else {
> -		submit_one_bio(bio, 0, 0);
> +		__submit_one_bio(&epd->bio_ctrl);
>   	}
> -
> -	/* The bio is owned by the bi_end_io handler now */
> -	epd->bio_ctrl.bio = NULL;
>   }
>
>   int __init extent_state_cache_init(void)
> @@ -3408,7 +3412,6 @@ static int submit_extent_page(unsigned int opf,
>   			      struct page *page, u64 disk_bytenr,
>   			      size_t size, unsigned long pg_offset,
>   			      bio_end_io_t end_io_func,
> -			      int mirror_num,
>   			      enum btrfs_compression_type compress_type,
>   			      bool force_bio_submit)
>   {
> @@ -3420,10 +3423,8 @@ static int submit_extent_page(unsigned int opf,
>
>   	ASSERT(pg_offset < PAGE_SIZE && size <= PAGE_SIZE &&
>   	       pg_offset + size <= PAGE_SIZE);
> -	if (force_bio_submit && bio_ctrl->bio) {
> -		submit_one_bio(bio_ctrl->bio, mirror_num, bio_ctrl->compress_type);
> -		bio_ctrl->bio = NULL;
> -	}
> +	if (force_bio_submit)
> +		submit_one_bio(bio_ctrl);
>
>   	while (cur < pg_offset + size) {
>   		u32 offset = cur - pg_offset;
> @@ -3463,8 +3464,7 @@ static int submit_extent_page(unsigned int opf,
>   		if (added < size - offset) {
>   			/* The bio should contain some page(s) */
>   			ASSERT(bio_ctrl->bio->bi_iter.bi_size);
> -			submit_one_bio(bio_ctrl->bio, mirror_num, bio_ctrl->compress_type);
> -			bio_ctrl->bio = NULL;
> +			submit_one_bio(bio_ctrl);
>   		}
>   		cur += added;
>   	}
> @@ -3741,10 +3741,8 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
>
>   		ret = submit_extent_page(REQ_OP_READ | read_flags, NULL,
>   					 bio_ctrl, page, disk_bytenr, iosize,
> -					 pg_offset,
> -					 end_bio_extent_readpage, 0,
> -					 this_bio_flag,
> -					 force_bio_submit);
> +					 pg_offset, end_bio_extent_readpage,
> +					 this_bio_flag, force_bio_submit);
>   		if (ret) {
>   			/*
>   			 * We have to unlock the remaining range, or the page
> @@ -3776,8 +3774,7 @@ int btrfs_readpage(struct file *file, struct page *page)
>   	 * If btrfs_do_readpage() failed we will want to submit the assembled
>   	 * bio to do the cleanup.
>   	 */
> -	if (bio_ctrl.bio)
> -		submit_one_bio(bio_ctrl.bio, 0, bio_ctrl.compress_type);
> +	submit_one_bio(&bio_ctrl);
>   	return ret;
>   }
>
> @@ -4060,7 +4057,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   					 disk_bytenr, iosize,
>   					 cur - page_offset(page),
>   					 end_bio_extent_writepage,
> -					 0, 0, false);
> +					 0, false);
>   		if (ret) {
>   			has_error = true;
>   			if (!saved_ret)
> @@ -4553,7 +4550,7 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
>   	ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc,
>   			&epd->bio_ctrl, page, eb->start, eb->len,
>   			eb->start - page_offset(page),
> -			end_bio_subpage_eb_writepage, 0, 0, false);
> +			end_bio_subpage_eb_writepage, 0, false);
>   	if (ret) {
>   		btrfs_subpage_clear_writeback(fs_info, page, eb->start, eb->len);
>   		set_btree_ioerr(page, eb);
> @@ -4594,7 +4591,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
>   					 &epd->bio_ctrl, p, disk_bytenr,
>   					 PAGE_SIZE, 0,
>   					 end_bio_extent_buffer_writepage,
> -					 0, 0, false);
> +					 0, false);
>   		if (ret) {
>   			set_btree_ioerr(p, eb);
>   			if (PageWriteback(p))
> @@ -5207,9 +5204,7 @@ void extent_readahead(struct readahead_control *rac)
>
>   	if (em_cached)
>   		free_extent_map(em_cached);
> -
> -	if (bio_ctrl.bio)
> -		submit_one_bio(bio_ctrl.bio, 0, bio_ctrl.compress_type);
> +	submit_one_bio(&bio_ctrl);
>   }
>
>   /*
> @@ -6536,7 +6531,9 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
>   	struct btrfs_fs_info *fs_info = eb->fs_info;
>   	struct extent_io_tree *io_tree;
>   	struct page *page = eb->pages[0];
> -	struct btrfs_bio_ctrl bio_ctrl = { 0 };
> +	struct btrfs_bio_ctrl bio_ctrl = {
> +		.mirror_num = mirror_num,
> +	};
>   	int ret = 0;
>
>   	ASSERT(!test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags));
> @@ -6571,8 +6568,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
>   	ret = submit_extent_page(REQ_OP_READ, NULL, &bio_ctrl,
>   				 page, eb->start, eb->len,
>   				 eb->start - page_offset(page),
> -				 end_bio_extent_readpage, mirror_num, 0,
> -				 true);
> +				 end_bio_extent_readpage, 0, true);
>   	if (ret) {
>   		/*
>   		 * In the endio function, if we hit something wrong we will
> @@ -6581,10 +6577,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
>   		 */
>   		atomic_dec(&eb->io_pages);
>   	}
> -	if (bio_ctrl.bio) {
> -		submit_one_bio(bio_ctrl.bio, mirror_num, 0);
> -		bio_ctrl.bio = NULL;
> -	}
> +	submit_one_bio(&bio_ctrl);
>   	if (ret || wait != WAIT_COMPLETE)
>   		return ret;
>
> @@ -6604,7 +6597,9 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>   	int all_uptodate = 1;
>   	int num_pages;
>   	unsigned long num_reads = 0;
> -	struct btrfs_bio_ctrl bio_ctrl = { 0 };
> +	struct btrfs_bio_ctrl bio_ctrl = {
> +		.mirror_num = mirror_num,
> +	};
>
>   	if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
>   		return 0;
> @@ -6678,7 +6673,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>   			err = submit_extent_page(REQ_OP_READ, NULL,
>   					 &bio_ctrl, page, page_offset(page),
>   					 PAGE_SIZE, 0, end_bio_extent_readpage,
> -					 mirror_num, 0, false);
> +					 0, false);
>   			if (err) {
>   				/*
>   				 * We failed to submit the bio so it's the
> @@ -6695,10 +6690,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>   		}
>   	}
>
> -	if (bio_ctrl.bio) {
> -		submit_one_bio(bio_ctrl.bio, mirror_num, bio_ctrl.compress_type);
> -		bio_ctrl.bio = NULL;
> -	}
> +	submit_one_bio(&bio_ctrl);
>
>   	if (ret || wait != WAIT_COMPLETE)
>   		return ret;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio
  2022-06-04 22:31   ` Qu Wenruo
@ 2022-06-06  6:30     ` Christoph Hellwig
  0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2022-06-06  6:30 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: Christoph Hellwig, Josef Bacik, David Sterba, linux-btrfs

On Sun, Jun 05, 2022 at 06:31:18AM +0800, Qu Wenruo wrote:
> In fact the only call sites really caring num_mirror is the metadata
> read path (as it doesn't rely on the read-repair code, since metadata
> has inline csum and it has a different validation condition).
>
> It may be a good idea to make a union for btrfs_bio, to contain all the
> needed info for metadata verification (btrfs_key, transid, level), so
> that we can get rid of the num_mirror parameter for submit_extent_page()
> completely.

My idea was to pass that in structure in bio->bi_private.  But that
is just thought for now, I've not tried to actually implement it yet.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio
  2022-06-03  7:11 ` [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio Christoph Hellwig
  2022-06-04 22:31   ` Qu Wenruo
@ 2022-06-06 10:41   ` Nikolay Borisov
  2022-06-06 16:29     ` Christoph Hellwig
  1 sibling, 1 reply; 12+ messages in thread
From: Nikolay Borisov @ 2022-06-06 10:41 UTC (permalink / raw)
  To: Christoph Hellwig, Josef Bacik, David Sterba; +Cc: linux-btrfs



On 3.06.22 г. 10:11 ч., Christoph Hellwig wrote:
> submit_one_bio always works on the bio and compression flags from a
> btrfs_bio_ctrl structure.  Pass the explicitly and clean up the the
> calling conventions by handling a NULL bio in submit_one_bio, and
> using the btrfs_bio_ctrl to pass the mirror number as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/extent_io.c | 82 ++++++++++++++++++++------------------------
>   1 file changed, 37 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 72a258fa27947..facca7feb9a22 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -144,6 +144,7 @@ struct tree_entry {
>    */
>   struct btrfs_bio_ctrl {
>   	struct bio *bio;
> +	int mirror_num;
>   	enum btrfs_compression_type compress_type;
>   	u32 len_to_stripe_boundary;
>   	u32 len_to_oe_boundary;
> @@ -178,10 +179,11 @@ static int add_extent_changeset(struct extent_state *state, u32 bits,
>   	return ret;
>   }
>   
> -static void submit_one_bio(struct bio *bio, int mirror_num,
> -			   enum btrfs_compression_type compress_type)
> +static void __submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
>   {
> +	struct bio *bio = bio_ctrl->bio;
>   	struct inode *inode = bio_first_page_all(bio)->mapping->host;
> +	int mirror_num = bio_ctrl->mirror_num;
>   
>   	/* Caller should ensure the bio has at least some range added */
>   	ASSERT(bio->bi_iter.bi_size);
> @@ -191,14 +193,17 @@ static void submit_one_bio(struct bio *bio, int mirror_num,
>   	else if (btrfs_op(bio) == BTRFS_MAP_WRITE)
>   		btrfs_submit_data_write_bio(inode, bio, mirror_num);
>   	else
> -		btrfs_submit_data_read_bio(inode, bio, mirror_num, compress_type);
> +		btrfs_submit_data_read_bio(inode, bio, mirror_num,
> +					   bio_ctrl->compress_type);
>   
> -	/*
> -	 * Above submission hooks will handle the error by ending the bio,
> -	 * which will do the cleanup properly.  So here we should not return
> -	 * any error, or the caller of submit_extent_page() will do cleanup
> -	 * again, causing problems.
> -	 */
> +	/* The bio is owned by the bi_end_io handler now */
> +	bio_ctrl->bio = NULL;
> +}
> +
> +static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
> +{
> +	if (bio_ctrl->bio)
> +		__submit_one_bio(bio_ctrl);
>   }

Why do you need a function just to put an if in it, just move this atop 
__submit_one_bio :

if (!bio_ctrl->bio)
     return

and rename it to submit_one_bio.

<snip>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio
  2022-06-06 10:41   ` Nikolay Borisov
@ 2022-06-06 16:29     ` Christoph Hellwig
  2022-06-06 20:23       ` David Sterba
  0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2022-06-06 16:29 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: Christoph Hellwig, Josef Bacik, David Sterba, linux-btrfs

On Mon, Jun 06, 2022 at 01:41:50PM +0300, Nikolay Borisov wrote:
>> +static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
>> +{
>> +	if (bio_ctrl->bio)
>> +		__submit_one_bio(bio_ctrl);
>>   }
>
> Why do you need a function just to put an if in it, just move this atop 
> __submit_one_bio :
>
> if (!bio_ctrl->bio)
>     return
>
> and rename it to submit_one_bio.

Because just moving it to the top will lead to null pointer dereferences.
I'd also have to move some initialization down.  Based on that the
wrapper seems cleaner and safer to me.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio
  2022-06-06 16:29     ` Christoph Hellwig
@ 2022-06-06 20:23       ` David Sterba
  0 siblings, 0 replies; 12+ messages in thread
From: David Sterba @ 2022-06-06 20:23 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Nikolay Borisov, Josef Bacik, David Sterba, linux-btrfs

On Mon, Jun 06, 2022 at 06:29:29PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 06, 2022 at 01:41:50PM +0300, Nikolay Borisov wrote:
> >> +static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
> >> +{
> >> +	if (bio_ctrl->bio)
> >> +		__submit_one_bio(bio_ctrl);
> >>   }
> >
> > Why do you need a function just to put an if in it, just move this atop 
> > __submit_one_bio :
> >
> > if (!bio_ctrl->bio)
> >     return
> >
> > and rename it to submit_one_bio.
> 
> Because just moving it to the top will lead to null pointer dereferences.
> I'd also have to move some initialization down.  Based on that the
> wrapper seems cleaner and safer to me.

I don't see much reason to have the safe and unsafe variants, it's all
for internal use in one file, also there's only one real instance where
__submit_one_bio can be used.  I'd expect a normal and __ variant for
some public API. Moving the initialization does not seem to be making
the code hard to read, so I'd apply this diff on top of your patch:

--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -179,11 +179,18 @@ static int add_extent_changeset(struct extent_state *state, u32 bits,
        return ret;
 }
 
-static void __submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
+static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
 {
-       struct bio *bio = bio_ctrl->bio;
-       struct inode *inode = bio_first_page_all(bio)->mapping->host;
-       int mirror_num = bio_ctrl->mirror_num;
+       struct bio *bio;
+       struct inode *inode;
+       int mirror_num;
+
+       if (!bio_ctrl->bio)
+               return;
+
+       bio = bio_ctrl->bio;
+       inode = bio_first_page_all(bio)->mapping->host;
+       mirror_num = bio_ctrl->mirror_num;
 
        /* Caller should ensure the bio has at least some range added */
        ASSERT(bio->bi_iter.bi_size);
@@ -200,12 +207,6 @@ static void __submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
        bio_ctrl->bio = NULL;
 }
 
-static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
-{
-       if (bio_ctrl->bio)
-               __submit_one_bio(bio_ctrl);
-}
-
 /*
  * Submit or fail the current bio in an extent_page_data structure.
  */
@@ -223,7 +224,7 @@ static void submit_write_bio(struct extent_page_data *epd, int ret)
                /* The bio is owned by the bi_end_io handler now */
                epd->bio_ctrl.bio = NULL;
        } else {
-               __submit_one_bio(&epd->bio_ctrl);
+               submit_one_bio(&epd->bio_ctrl);
        }
 }
---

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: extent_io bio submission cleanup
  2022-06-03  7:11 extent_io bio submission cleanup Christoph Hellwig
                   ` (2 preceding siblings ...)
  2022-06-03  7:11 ` [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio Christoph Hellwig
@ 2022-06-06 20:26 ` David Sterba
  3 siblings, 0 replies; 12+ messages in thread
From: David Sterba @ 2022-06-06 20:26 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Josef Bacik, David Sterba, linux-btrfs

On Fri, Jun 03, 2022 at 09:11:00AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series cleans up how extent_io.c submits bios.

Thanks, added to misc-next, for now with the fixup as a separate patch
so it's clear in case there's an error but I intend to merge it in to
the third patch.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-06-06 20:33 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-03  7:11 extent_io bio submission cleanup Christoph Hellwig
2022-06-03  7:11 ` [PATCH 1/3] btrfs: don't use bio->bi_private to pass the inode to submit_one_bio Christoph Hellwig
2022-06-03 10:33   ` Qu Wenruo
2022-06-03  7:11 ` [PATCH 2/3] btrfs: merge end_write_bio and flush_write_bio Christoph Hellwig
2022-06-03 10:40   ` Qu Wenruo
2022-06-03  7:11 ` [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio Christoph Hellwig
2022-06-04 22:31   ` Qu Wenruo
2022-06-06  6:30     ` Christoph Hellwig
2022-06-06 10:41   ` Nikolay Borisov
2022-06-06 16:29     ` Christoph Hellwig
2022-06-06 20:23       ` David Sterba
2022-06-06 20:26 ` extent_io bio submission cleanup David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.