minor bio submission cleanups

minor bio submission cleanups

[Christoph Hellwig]

Hi all,

this series cleans up a few loose ends in the btrfs bio submission path.

Diffstat:
 compression.c |   11 +++++------
 compression.h |    4 ++--
 ctree.h       |    5 ++---
 disk-io.c     |   26 ++++++++++----------------
 disk-io.h     |    4 ++--
 extent_io.c   |   39 +++++++++++++++++++++++++++++++++++----
 extent_io.h   |   18 ++----------------
 inode.c       |   49 ++++++++++---------------------------------------
 8 files changed, 68 insertions(+), 88 deletions(-)

[PATCH 1/5] btrfs: move btrfs_readpage to extent_io.c

[Christoph Hellwig]

Keep btrfs_readpage next to btrfs_do_readpage and the other address
space operations.  This allows to keep submit_one_bio and
struct btrfs_bio_ctrl file local in extent_io.c.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/ctree.h     |  1 -
 fs/btrfs/extent_io.c | 36 ++++++++++++++++++++++++++++++++++--
 fs/btrfs/extent_io.h | 16 +---------------
 fs/btrfs/inode.c     | 20 --------------------
 4 files changed, 35 insertions(+), 38 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 55dee124ee447..0fd3a21cd5a89 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3312,7 +3312,6 @@ void btrfs_split_delalloc_extent(struct inode *inode,
 				 struct extent_state *orig, u64 split);
 void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end);
 vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf);
-int btrfs_readpage(struct file *file, struct page *page);
 void btrfs_evict_inode(struct inode *inode);
 int btrfs_write_inode(struct inode *inode, struct writeback_control *wbc);
 struct inode *btrfs_alloc_inode(struct super_block *sb);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 779123e68d7b6..fe146ba21415e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -137,6 +137,17 @@ struct tree_entry {
 	struct rb_node rb_node;
 };
 
+/*
+ * Structure to record info about the bio being assembled, and other info like
+ * how many bytes are there before stripe/ordered extent boundary.
+ */
+struct btrfs_bio_ctrl {
+	struct bio *bio;
+	unsigned long bio_flags;
+	u32 len_to_stripe_boundary;
+	u32 len_to_oe_boundary;
+};
+
 struct extent_page_data {
 	struct btrfs_bio_ctrl bio_ctrl;
 	/* tells writepage not to lock the state bits for this range
@@ -166,7 +177,8 @@ static int add_extent_changeset(struct extent_state *state, u32 bits,
 	return ret;
 }
 
-void submit_one_bio(struct bio *bio, int mirror_num, unsigned long bio_flags)
+static void submit_one_bio(struct bio *bio, int mirror_num,
+		unsigned long bio_flags)
 {
 	struct extent_io_tree *tree = bio->bi_private;
 
@@ -3604,7 +3616,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
  * XXX JDM: This needs looking at to ensure proper page locking
  * return 0 on success, otherwise return error
  */
-int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
+static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 		      struct btrfs_bio_ctrl *bio_ctrl,
 		      unsigned int read_flags, u64 *prev_em_start)
 {
@@ -3793,6 +3805,26 @@ int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 	return ret;
 }
 
+int btrfs_readpage(struct file *file, struct page *page)
+{
+	struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
+	u64 start = page_offset(page);
+	u64 end = start + PAGE_SIZE - 1;
+	struct btrfs_bio_ctrl bio_ctrl = { 0 };
+	int ret;
+
+	btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
+
+	ret = btrfs_do_readpage(page, NULL, &bio_ctrl, 0, NULL);
+	/*
+	 * 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.bio_flags);
+	return ret;
+}
+
 static inline void contiguous_readpages(struct page *pages[], int nr_pages,
 					u64 start, u64 end,
 					struct extent_map **em_cached,
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 9a283b2358b83..c94df8e2ab9d6 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -102,17 +102,6 @@ struct extent_buffer {
 #endif
 };
 
-/*
- * Structure to record info about the bio being assembled, and other info like
- * how many bytes are there before stripe/ordered extent boundary.
- */
-struct btrfs_bio_ctrl {
-	struct bio *bio;
-	unsigned long bio_flags;
-	u32 len_to_stripe_boundary;
-	u32 len_to_oe_boundary;
-};
-
 /*
  * Structure to record how many bytes and which ranges are set/cleared
  */
@@ -178,10 +167,7 @@ typedef struct extent_map *(get_extent_t)(struct btrfs_inode *inode,
 int try_release_extent_mapping(struct page *page, gfp_t mask);
 int try_release_extent_buffer(struct page *page);
 
-void submit_one_bio(struct bio *bio, int mirror_num, unsigned long bio_flags);
-int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
-		      struct btrfs_bio_ctrl *bio_ctrl,
-		      unsigned int read_flags, u64 *prev_em_start);
+int btrfs_readpage(struct file *file, struct page *page);
 int extent_write_full_page(struct page *page, struct writeback_control *wbc);
 int extent_write_locked_range(struct inode *inode, u64 start, u64 end);
 int extent_writepages(struct address_space *mapping,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 29879fb257d5d..f2fb2bfc2f9a2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8147,26 +8147,6 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	return extent_fiemap(BTRFS_I(inode), fieinfo, start, len);
 }
 
-int btrfs_readpage(struct file *file, struct page *page)
-{
-	struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
-	u64 start = page_offset(page);
-	u64 end = start + PAGE_SIZE - 1;
-	struct btrfs_bio_ctrl bio_ctrl = { 0 };
-	int ret;
-
-	btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
-
-	ret = btrfs_do_readpage(page, NULL, &bio_ctrl, 0, NULL);
-	/*
-	 * 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.bio_flags);
-	return ret;
-}
-
 static int btrfs_writepage(struct page *page, struct writeback_control *wbc)
 {
 	struct inode *inode = page->mapping->host;
-- 
2.30.2

[PATCH 2/5] btrfs: remove the unused bio_flags argument to btrfs_submit_metadata_bio

[Christoph Hellwig]

This argument is unused since commit 953651eb308f ("btrfs: factor out
helper adding a page to bio") and commit 1b36294a6cd5 ("btrfs: call
submit_bio_hook directly for metadata pages") reworked the way metadata
bio submission is handled.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/disk-io.c   | 2 +-
 fs/btrfs/disk-io.h   | 2 +-
 fs/btrfs/extent_io.c | 3 +--
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2689e85896272..28b9cf020b8df 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -921,7 +921,7 @@ static bool should_async_write(struct btrfs_fs_info *fs_info,
 }
 
 blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
-				       int mirror_num, unsigned long bio_flags)
+				       int mirror_num)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	blk_status_t ret;
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 2a401592124d3..56607abe75aa4 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -88,7 +88,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_bio *bbio,
 				   struct page *page, u64 start, u64 end,
 				   int mirror);
 blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
-				       int mirror_num, unsigned long bio_flags);
+				       int mirror_num);
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 struct btrfs_root *btrfs_alloc_dummy_root(struct btrfs_fs_info *fs_info);
 #endif
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index fe146ba21415e..ef3f77e0b0fed 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -191,8 +191,7 @@ static void submit_one_bio(struct bio *bio, int mirror_num,
 		btrfs_submit_data_bio(tree->private_data, bio, mirror_num,
 					    bio_flags);
 	else
-		btrfs_submit_metadata_bio(tree->private_data, bio,
-						mirror_num, bio_flags);
+		btrfs_submit_metadata_bio(tree->private_data, bio, mirror_num);
 	/*
 	 * Above submission hooks will handle the error by ending the bio,
 	 * which will do the cleanup properly.  So here we should not return
-- 
2.30.2

[PATCH 3/5] btrfs: do not return errors from btrfs_submit_metadata_bio

[Christoph Hellwig]

btrfs_submit_metadata_bio already calls ->bi_end_io on error and the
caller must ignore the return value, so remove it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/disk-io.c | 26 ++++++++++----------------
 fs/btrfs/disk-io.h |  4 ++--
 2 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 28b9cf020b8df..9c488224edc73 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -920,8 +920,8 @@ static bool should_async_write(struct btrfs_fs_info *fs_info,
 	return true;
 }
 
-blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
-				       int mirror_num)
+void btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
+			       int mirror_num)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	blk_status_t ret;
@@ -933,14 +933,12 @@ blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
 		 */
 		ret = btrfs_bio_wq_end_io(fs_info, bio,
 					  BTRFS_WQ_ENDIO_METADATA);
-		if (ret)
-			goto out_w_error;
-		ret = btrfs_map_bio(fs_info, bio, mirror_num);
+		if (!ret)
+			ret = btrfs_map_bio(fs_info, bio, mirror_num);
 	} else if (!should_async_write(fs_info, BTRFS_I(inode))) {
 		ret = btree_csum_one_bio(bio);
-		if (ret)
-			goto out_w_error;
-		ret = btrfs_map_bio(fs_info, bio, mirror_num);
+		if (!ret)
+			ret = btrfs_map_bio(fs_info, bio, mirror_num);
 	} else {
 		/*
 		 * kthread helpers are used to submit writes so that
@@ -950,14 +948,10 @@ blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
 					  0, btree_submit_bio_start);
 	}
 
-	if (ret)
-		goto out_w_error;
-	return 0;
-
-out_w_error:
-	bio->bi_status = ret;
-	bio_endio(bio);
-	return ret;
+	if (ret) {
+		bio->bi_status = ret;
+		bio_endio(bio);
+	}
 }
 
 #ifdef CONFIG_MIGRATION
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 56607abe75aa4..1312d93c02edb 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -87,8 +87,8 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
 int btrfs_validate_metadata_buffer(struct btrfs_bio *bbio,
 				   struct page *page, u64 start, u64 end,
 				   int mirror);
-blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
-				       int mirror_num);
+void btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
+			       int mirror_num);
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 struct btrfs_root *btrfs_alloc_dummy_root(struct btrfs_fs_info *fs_info);
 #endif
-- 
2.30.2

[PATCH 4/5] btrfs: do not return errors from btrfs_submit_compressed_read

[Christoph Hellwig]

btrfs_submit_compressed_read already calls ->bi_end_io on error and
the caller must ignore the return value, so remove it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/compression.c | 11 +++++------
 fs/btrfs/compression.h |  4 ++--
 fs/btrfs/inode.c       |  8 +++-----
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 3e8417bfabe65..8fda38a587067 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -801,8 +801,8 @@ static noinline int add_ra_bio_pages(struct inode *inode,
  * After the compressed pages are read, we copy the bytes into the
  * bio we were passed and then call the bio end_io calls
  */
-blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
-				 int mirror_num, unsigned long bio_flags)
+void btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
+				  int mirror_num, unsigned long bio_flags)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct extent_map_tree *em_tree;
@@ -947,7 +947,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 			comp_bio = NULL;
 		}
 	}
-	return BLK_STS_OK;
+	return;
 
 fail:
 	if (cb->compressed_pages) {
@@ -963,7 +963,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	free_extent_map(em);
 	bio->bi_status = ret;
 	bio_endio(bio);
-	return ret;
+	return;
 finish_cb:
 	if (comp_bio) {
 		comp_bio->bi_status = ret;
@@ -971,7 +971,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	}
 	/* All bytes of @cb is submitted, endio will free @cb */
 	if (cur_disk_byte == disk_bytenr + compressed_len)
-		return ret;
+		return;
 
 	wait_var_event(cb, refcount_read(&cb->pending_sectors) ==
 			   (disk_bytenr + compressed_len - cur_disk_byte) >>
@@ -983,7 +983,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	ASSERT(refcount_read(&cb->pending_sectors));
 	/* Now we are the only one referring @cb, can finish it safely. */
 	finish_compressed_bio_read(cb);
-	return ret;
 }
 
 /*
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index ac5b20731d2ad..ac3c79f8c3492 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -102,8 +102,8 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 				  unsigned int write_flags,
 				  struct cgroup_subsys_state *blkcg_css,
 				  bool writeback);
-blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
-				 int mirror_num, unsigned long bio_flags);
+void btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
+				  int mirror_num, unsigned long bio_flags);
 
 unsigned int btrfs_compress_str2level(unsigned int type, const char *str);
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f2fb2bfc2f9a2..414156c0ac38a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2618,10 +2618,9 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
 			 * the bio if there were any errors, so just return
 			 * here.
 			 */
-			ret = btrfs_submit_compressed_read(inode, bio,
-							   mirror_num,
-							   bio_flags);
-			goto out_no_endio;
+			btrfs_submit_compressed_read(inode, bio, mirror_num,
+						     bio_flags);
+			return BLK_STS_OK;
 		} else {
 			/*
 			 * Lookup bio sums does extra checks around whether we
@@ -2655,7 +2654,6 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
 		bio->bi_status = ret;
 		bio_endio(bio);
 	}
-out_no_endio:
 	return ret;
 }
 
-- 
2.30.2

[PATCH 5/5] btrfs: do not return errors from submit_bio_hook_t instances

[Christoph Hellwig]

Both btrfs_repair_one_sector and submit_bio_one as the direct caller of
one of the instances ignore errors as they expect the methods themselves
to call ->bi_end_io on error.  Remove the unused and dangerous return
value.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/ctree.h     |  4 ++--
 fs/btrfs/extent_io.h |  2 +-
 fs/btrfs/inode.c     | 23 ++++++++---------------
 3 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0fd3a21cd5a89..67e169ba55e87 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3249,8 +3249,8 @@ void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode, u64 new_i_siz
 u64 btrfs_file_extent_end(const struct btrfs_path *path);
 
 /* inode.c */
-blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
-				   int mirror_num, unsigned long bio_flags);
+void btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
+			   int mirror_num, unsigned long bio_flags);
 unsigned int btrfs_verify_data_csum(struct btrfs_bio *bbio,
 				    u32 bio_offset, struct page *page,
 				    u64 start, u64 end);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index c94df8e2ab9d6..b390ec79f9a86 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -71,7 +71,7 @@ struct btrfs_fs_info;
 struct io_failure_record;
 struct extent_io_tree;
 
-typedef blk_status_t (submit_bio_hook_t)(struct inode *inode, struct bio *bio,
+typedef void (submit_bio_hook_t)(struct inode *inode, struct bio *bio,
 					 int mirror_num,
 					 unsigned long bio_flags);
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 414156c0ac38a..a37da2decf958 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2581,9 +2581,8 @@ static blk_status_t extract_ordered_extent(struct btrfs_inode *inode,
  *
  *    c-3) otherwise:			async submit
  */
-blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
-				   int mirror_num, unsigned long bio_flags)
-
+void btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
+			   int mirror_num, unsigned long bio_flags)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
@@ -2620,7 +2619,7 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
 			 */
 			btrfs_submit_compressed_read(inode, bio, mirror_num,
 						     bio_flags);
-			return BLK_STS_OK;
+			return;
 		} else {
 			/*
 			 * Lookup bio sums does extra checks around whether we
@@ -2654,7 +2653,6 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
 		bio->bi_status = ret;
 		bio_endio(bio);
 	}
-	return ret;
 }
 
 /*
@@ -7798,25 +7796,20 @@ static void btrfs_dio_private_put(struct btrfs_dio_private *dip)
 	kfree(dip);
 }
 
-static blk_status_t submit_dio_repair_bio(struct inode *inode, struct bio *bio,
-					  int mirror_num,
-					  unsigned long bio_flags)
+static void submit_dio_repair_bio(struct inode *inode, struct bio *bio,
+				  int mirror_num, unsigned long bio_flags)
 {
 	struct btrfs_dio_private *dip = bio->bi_private;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	blk_status_t ret;
 
 	BUG_ON(bio_op(bio) == REQ_OP_WRITE);
 
-	ret = btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA);
-	if (ret)
-		return ret;
+	if (btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA))
+		return;
 
 	refcount_inc(&dip->refs);
-	ret = btrfs_map_bio(fs_info, bio, mirror_num);
-	if (ret)
+	if (btrfs_map_bio(fs_info, bio, mirror_num))
 		refcount_dec(&dip->refs);
-	return ret;
 }
 
 static blk_status_t btrfs_check_read_dio_bio(struct btrfs_dio_private *dip,
-- 
2.30.2

Re: [PATCH 2/5] btrfs: remove the unused bio_flags argument to btrfs_submit_metadata_bio

[Qu Wenruo]

On 2022/4/15 22:33, Christoph Hellwig wrote:
> This argument is unused since commit 953651eb308f ("btrfs: factor out
> helper adding a page to bio") and commit 1b36294a6cd5 ("btrfs: call
> submit_bio_hook directly for metadata pages") reworked the way metadata
> bio submission is handled.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/disk-io.c   | 2 +-
>   fs/btrfs/disk-io.h   | 2 +-
>   fs/btrfs/extent_io.c | 3 +--
>   3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 2689e85896272..28b9cf020b8df 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -921,7 +921,7 @@ static bool should_async_write(struct btrfs_fs_info *fs_info,
>   }
>
>   blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
> -				       int mirror_num, unsigned long bio_flags)
> +				       int mirror_num)
>   {
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   	blk_status_t ret;
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index 2a401592124d3..56607abe75aa4 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -88,7 +88,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_bio *bbio,
>   				   struct page *page, u64 start, u64 end,
>   				   int mirror);
>   blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
> -				       int mirror_num, unsigned long bio_flags);
> +				       int mirror_num);
>   #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>   struct btrfs_root *btrfs_alloc_dummy_root(struct btrfs_fs_info *fs_info);
>   #endif
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index fe146ba21415e..ef3f77e0b0fed 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -191,8 +191,7 @@ static void submit_one_bio(struct bio *bio, int mirror_num,
>   		btrfs_submit_data_bio(tree->private_data, bio, mirror_num,
>   					    bio_flags);
>   	else
> -		btrfs_submit_metadata_bio(tree->private_data, bio,
> -						mirror_num, bio_flags);
> +		btrfs_submit_metadata_bio(tree->private_data, bio, mirror_num);
>   	/*
>   	 * Above submission hooks will handle the error by ending the bio,
>   	 * which will do the cleanup properly.  So here we should not return

Re: [PATCH 3/5] btrfs: do not return errors from btrfs_submit_metadata_bio

[Qu Wenruo]

On 2022/4/15 22:33, Christoph Hellwig wrote:
> btrfs_submit_metadata_bio already calls ->bi_end_io on error and the
> caller must ignore the return value, so remove it.

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

I think your cleanup is doing the first step towards a proper defined
behavior on how we should handle bios: just fire and forget.

Thanks,
Qu
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/disk-io.c | 26 ++++++++++----------------
>   fs/btrfs/disk-io.h |  4 ++--
>   2 files changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 28b9cf020b8df..9c488224edc73 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -920,8 +920,8 @@ static bool should_async_write(struct btrfs_fs_info *fs_info,
>   	return true;
>   }
>
> -blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
> -				       int mirror_num)
> +void btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
> +			       int mirror_num)
>   {
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   	blk_status_t ret;
> @@ -933,14 +933,12 @@ blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
>   		 */
>   		ret = btrfs_bio_wq_end_io(fs_info, bio,
>   					  BTRFS_WQ_ENDIO_METADATA);
> -		if (ret)
> -			goto out_w_error;
> -		ret = btrfs_map_bio(fs_info, bio, mirror_num);
> +		if (!ret)
> +			ret = btrfs_map_bio(fs_info, bio, mirror_num);
>   	} else if (!should_async_write(fs_info, BTRFS_I(inode))) {
>   		ret = btree_csum_one_bio(bio);
> -		if (ret)
> -			goto out_w_error;
> -		ret = btrfs_map_bio(fs_info, bio, mirror_num);
> +		if (!ret)
> +			ret = btrfs_map_bio(fs_info, bio, mirror_num);
>   	} else {
>   		/*
>   		 * kthread helpers are used to submit writes so that
> @@ -950,14 +948,10 @@ blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
>   					  0, btree_submit_bio_start);
>   	}
>
> -	if (ret)
> -		goto out_w_error;
> -	return 0;
> -
> -out_w_error:
> -	bio->bi_status = ret;
> -	bio_endio(bio);
> -	return ret;
> +	if (ret) {
> +		bio->bi_status = ret;
> +		bio_endio(bio);
> +	}
>   }
>
>   #ifdef CONFIG_MIGRATION
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index 56607abe75aa4..1312d93c02edb 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -87,8 +87,8 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
>   int btrfs_validate_metadata_buffer(struct btrfs_bio *bbio,
>   				   struct page *page, u64 start, u64 end,
>   				   int mirror);
> -blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
> -				       int mirror_num);
> +void btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
> +			       int mirror_num);
>   #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>   struct btrfs_root *btrfs_alloc_dummy_root(struct btrfs_fs_info *fs_info);
>   #endif

Re: minor bio submission cleanups

[Qu Wenruo]

On 2022/4/15 22:33, Christoph Hellwig wrote:
> Hi all,
>
> this series cleans up a few loose ends in the btrfs bio submission path.

Mind to share which commit your code are based on?

After patch 3/5 "btrfs: do not return errors from
btrfs_submit_metadata_bio", I was expecting the same cleanup for
btrfs_submit_data_bio(), but it doesn't.

Thus I guess it already handled by other patches in your branch?

Thanks,
Qu
>
> Diffstat:
>   compression.c |   11 +++++------
>   compression.h |    4 ++--
>   ctree.h       |    5 ++---
>   disk-io.c     |   26 ++++++++++----------------
>   disk-io.h     |    4 ++--
>   extent_io.c   |   39 +++++++++++++++++++++++++++++++++++----
>   extent_io.h   |   18 ++----------------
>   inode.c       |   49 ++++++++++---------------------------------------
>   8 files changed, 68 insertions(+), 88 deletions(-)

Re: [PATCH 4/5] btrfs: do not return errors from btrfs_submit_compressed_read

[Qu Wenruo]

On 2022/4/15 22:33, Christoph Hellwig wrote:
> btrfs_submit_compressed_read already calls ->bi_end_io on error and
> the caller must ignore the return value, so remove it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

The patch itself is fine.

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

Although more such cleanups are going to bring some question on how
those bio handling functions should behavior.

More and more bio submit functions are returning void and endio of the bio.

But there are still quite some not doing this, like btrfs_map_bio().

I'm wondering at which boundary we should return void and handle
everything in-house?

Thanks,
Qu
> ---
>   fs/btrfs/compression.c | 11 +++++------
>   fs/btrfs/compression.h |  4 ++--
>   fs/btrfs/inode.c       |  8 +++-----
>   3 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 3e8417bfabe65..8fda38a587067 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -801,8 +801,8 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>    * After the compressed pages are read, we copy the bytes into the
>    * bio we were passed and then call the bio end_io calls
>    */
> -blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> -				 int mirror_num, unsigned long bio_flags)
> +void btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> +				  int mirror_num, unsigned long bio_flags)
>   {
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   	struct extent_map_tree *em_tree;
> @@ -947,7 +947,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>   			comp_bio = NULL;
>   		}
>   	}
> -	return BLK_STS_OK;
> +	return;
>
>   fail:
>   	if (cb->compressed_pages) {
> @@ -963,7 +963,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>   	free_extent_map(em);
>   	bio->bi_status = ret;
>   	bio_endio(bio);
> -	return ret;
> +	return;
>   finish_cb:
>   	if (comp_bio) {
>   		comp_bio->bi_status = ret;
> @@ -971,7 +971,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>   	}
>   	/* All bytes of @cb is submitted, endio will free @cb */
>   	if (cur_disk_byte == disk_bytenr + compressed_len)
> -		return ret;
> +		return;
>
>   	wait_var_event(cb, refcount_read(&cb->pending_sectors) ==
>   			   (disk_bytenr + compressed_len - cur_disk_byte) >>
> @@ -983,7 +983,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>   	ASSERT(refcount_read(&cb->pending_sectors));
>   	/* Now we are the only one referring @cb, can finish it safely. */
>   	finish_compressed_bio_read(cb);
> -	return ret;
>   }
>
>   /*
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index ac5b20731d2ad..ac3c79f8c3492 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -102,8 +102,8 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
>   				  unsigned int write_flags,
>   				  struct cgroup_subsys_state *blkcg_css,
>   				  bool writeback);
> -blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> -				 int mirror_num, unsigned long bio_flags);
> +void btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> +				  int mirror_num, unsigned long bio_flags);
>
>   unsigned int btrfs_compress_str2level(unsigned int type, const char *str);
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index f2fb2bfc2f9a2..414156c0ac38a 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2618,10 +2618,9 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>   			 * the bio if there were any errors, so just return
>   			 * here.
>   			 */
> -			ret = btrfs_submit_compressed_read(inode, bio,
> -							   mirror_num,
> -							   bio_flags);
> -			goto out_no_endio;
> +			btrfs_submit_compressed_read(inode, bio, mirror_num,
> +						     bio_flags);
> +			return BLK_STS_OK;
>   		} else {
>   			/*
>   			 * Lookup bio sums does extra checks around whether we
> @@ -2655,7 +2654,6 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>   		bio->bi_status = ret;
>   		bio_endio(bio);
>   	}
> -out_no_endio:
>   	return ret;
>   }
>

Re: [PATCH 5/5] btrfs: do not return errors from submit_bio_hook_t instances

[Qu Wenruo]

On 2022/4/15 22:33, Christoph Hellwig wrote:
> Both btrfs_repair_one_sector and submit_bio_one as the direct caller of
> one of the instances ignore errors as they expect the methods themselves
> to call ->bi_end_io on error.  Remove the unused and dangerous return
> value.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/ctree.h     |  4 ++--
>   fs/btrfs/extent_io.h |  2 +-
>   fs/btrfs/inode.c     | 23 ++++++++---------------
>   3 files changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 0fd3a21cd5a89..67e169ba55e87 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3249,8 +3249,8 @@ void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode, u64 new_i_siz
>   u64 btrfs_file_extent_end(const struct btrfs_path *path);
>
>   /* inode.c */
> -blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
> -				   int mirror_num, unsigned long bio_flags);
> +void btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
> +			   int mirror_num, unsigned long bio_flags);
>   unsigned int btrfs_verify_data_csum(struct btrfs_bio *bbio,
>   				    u32 bio_offset, struct page *page,
>   				    u64 start, u64 end);
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index c94df8e2ab9d6..b390ec79f9a86 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -71,7 +71,7 @@ struct btrfs_fs_info;
>   struct io_failure_record;
>   struct extent_io_tree;
>
> -typedef blk_status_t (submit_bio_hook_t)(struct inode *inode, struct bio *bio,
> +typedef void (submit_bio_hook_t)(struct inode *inode, struct bio *bio,
>   					 int mirror_num,
>   					 unsigned long bio_flags);
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 414156c0ac38a..a37da2decf958 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2581,9 +2581,8 @@ static blk_status_t extract_ordered_extent(struct btrfs_inode *inode,
>    *
>    *    c-3) otherwise:			async submit
>    */
> -blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
> -				   int mirror_num, unsigned long bio_flags)
> -
> +void btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
> +			   int mirror_num, unsigned long bio_flags)
>   {
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   	struct btrfs_root *root = BTRFS_I(inode)->root;
> @@ -2620,7 +2619,7 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>   			 */
>   			btrfs_submit_compressed_read(inode, bio, mirror_num,
>   						     bio_flags);
> -			return BLK_STS_OK;
> +			return;
>   		} else {
>   			/*
>   			 * Lookup bio sums does extra checks around whether we
> @@ -2654,7 +2653,6 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>   		bio->bi_status = ret;
>   		bio_endio(bio);
>   	}
> -	return ret;
>   }
>
>   /*
> @@ -7798,25 +7796,20 @@ static void btrfs_dio_private_put(struct btrfs_dio_private *dip)
>   	kfree(dip);
>   }
>
> -static blk_status_t submit_dio_repair_bio(struct inode *inode, struct bio *bio,
> -					  int mirror_num,
> -					  unsigned long bio_flags)
> +static void submit_dio_repair_bio(struct inode *inode, struct bio *bio,
> +				  int mirror_num, unsigned long bio_flags)
>   {
>   	struct btrfs_dio_private *dip = bio->bi_private;
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> -	blk_status_t ret;
>
>   	BUG_ON(bio_op(bio) == REQ_OP_WRITE);
>
> -	ret = btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA);
> -	if (ret)
> -		return ret;
> +	if (btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA))
> +		return;
>
>   	refcount_inc(&dip->refs);
> -	ret = btrfs_map_bio(fs_info, bio, mirror_num);
> -	if (ret)
> +	if (btrfs_map_bio(fs_info, bio, mirror_num))
>   		refcount_dec(&dip->refs);
> -	return ret;
>   }
>
>   static blk_status_t btrfs_check_read_dio_bio(struct btrfs_dio_private *dip,

Re: minor bio submission cleanups

[Christoph Hellwig]

On Sat, Apr 16, 2022 at 06:44:05AM +0800, Qu Wenruo wrote:
> Mind to share which commit your code are based on?

the misc-next branch as of yesterday.

>
> After patch 3/5 "btrfs: do not return errors from
> btrfs_submit_metadata_bio", I was expecting the same cleanup for
> btrfs_submit_data_bio(), but it doesn't.

patch 5 handles it, but it btrfs_submit_data_bio can't be done standalone
as it is also used as a callback for btrfs_repair_one_sector.

Re: [PATCH 4/5] btrfs: do not return errors from btrfs_submit_compressed_read

[Christoph Hellwig]

On Sat, Apr 16, 2022 at 06:48:37AM +0800, Qu Wenruo wrote:
> More and more bio submit functions are returning void and endio of the bio.
>
> But there are still quite some not doing this, like btrfs_map_bio().
>
> I'm wondering at which boundary we should return void and handle
> everything in-house?

I don't think it is quite clear.  All the I/O errors with a bio should
be handled through end_io, and we already have that, module the compressed
case with it's extra layer of bios.  Now at what point we call the
endio handler is a different question.  Duplicating it everywhere is
a bit annoying, so having some low-level helpers that just return an
error might be useful.  I plan a fair amount of refactoring around
btrfs_map_bio, so I'll see if lifting the end_io call into it might
or might no make sense while I'm at it.

Re: [PATCH 4/5] btrfs: do not return errors from btrfs_submit_compressed_read

[Qu Wenruo]

On 2022/4/16 12:49, Christoph Hellwig wrote:
> On Sat, Apr 16, 2022 at 06:48:37AM +0800, Qu Wenruo wrote:
>> More and more bio submit functions are returning void and endio of the bio.
>>
>> But there are still quite some not doing this, like btrfs_map_bio().
>>
>> I'm wondering at which boundary we should return void and handle
>> everything in-house?
>
> I don't think it is quite clear.  All the I/O errors with a bio should
> be handled through end_io, and we already have that, module the compressed
> case with it's extra layer of bios.  Now at what point we call the
> endio handler is a different question.  Duplicating it everywhere is
> a bit annoying, so having some low-level helpers that just return an
> error might be useful.  I plan a fair amount of refactoring around
> btrfs_map_bio, so I'll see if lifting the end_io call into it might
> or might no make sense while I'm at it.

Great to know that.

Thanks,
Qu

Re: minor bio submission cleanups

[Nikolay Borisov]

On 15.04.22 г. 17:33 ч., Christoph Hellwig wrote:
> Hi all,
> 
> this series cleans up a few loose ends in the btrfs bio submission path.
> 
> Diffstat:
>   compression.c |   11 +++++------
>   compression.h |    4 ++--
>   ctree.h       |    5 ++---
>   disk-io.c     |   26 ++++++++++----------------
>   disk-io.h     |    4 ++--
>   extent_io.c   |   39 +++++++++++++++++++++++++++++++++++----
>   extent_io.h   |   18 ++----------------
>   inode.c       |   49 ++++++++++---------------------------------------
>   8 files changed, 68 insertions(+), 88 deletions(-)
> 

For the whole series:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: minor bio submission cleanups

[David Sterba]

On Fri, Apr 15, 2022 at 04:33:23PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series cleans up a few loose ends in the btrfs bio submission path.
> 
> Diffstat:
>  compression.c |   11 +++++------
>  compression.h |    4 ++--
>  ctree.h       |    5 ++---
>  disk-io.c     |   26 ++++++++++----------------
>  disk-io.h     |    4 ++--
>  extent_io.c   |   39 +++++++++++++++++++++++++++++++++++----
>  extent_io.h   |   18 ++----------------
>  inode.c       |   49 ++++++++++---------------------------------------
>  8 files changed, 68 insertions(+), 88 deletions(-)

Added to misc-next, thanks.

Re: [PATCH 4/5] btrfs: do not return errors from btrfs_submit_compressed_read

[David Sterba]

On Sat, Apr 16, 2022 at 06:49:20AM +0200, Christoph Hellwig wrote:
> On Sat, Apr 16, 2022 at 06:48:37AM +0800, Qu Wenruo wrote:
> > More and more bio submit functions are returning void and endio of the bio.
> >
> > But there are still quite some not doing this, like btrfs_map_bio().
> >
> > I'm wondering at which boundary we should return void and handle
> > everything in-house?
> 
> I don't think it is quite clear.  All the I/O errors with a bio should
> be handled through end_io, and we already have that, module the compressed
> case with it's extra layer of bios.  Now at what point we call the
> endio handler is a different question.  Duplicating it everywhere is
> a bit annoying, so having some low-level helpers that just return an
> error might be useful.  I plan a fair amount of refactoring around
> btrfs_map_bio, so I'll see if lifting the end_io call into it might
> or might no make sense while I'm at it.

Please try to limit number of patches in a series to 10ish, this works
much better when there's something to change in the early patches or
everything is fine and it will get merged for testing. Also there are
always other patches in flux so this helps me to schedule. Thanks.