small raid56 cleanups v3

small raid56 cleanups v3

[Christoph Hellwig]

Hi all,

this series has a few trivial code cleanups for the raid56 code while
I looked at it for another project.

Changes since v2:
 - lots of reshuffling based on feedback from qu

Changes since v1:
 - cleanup more of the work handlers
 - cleanup cleaning up unused bios

Diffstat:
 raid56.c |  324 ++++++++++++++++++++++-----------------------------------------
 1 file changed, 114 insertions(+), 210 deletions(-)

[PATCH 01/10] btrfs: cleanup raid56_parity_write

[Christoph Hellwig]

Handle the error return on alloc_rbio failure directly instead of using
a goto, and remove the queue_rbio goto label by moving the plugged
check into the if branch.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/raid56.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 6a2cf754912df2..2cd5b1d7983009 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1660,12 +1660,12 @@ void raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc)
 	struct btrfs_raid_bio *rbio;
 	struct btrfs_plug_cb *plug = NULL;
 	struct blk_plug_cb *cb;
-	int ret = 0;
 
 	rbio = alloc_rbio(fs_info, bioc);
 	if (IS_ERR(rbio)) {
-		ret = PTR_ERR(rbio);
-		goto fail;
+		bio->bi_status = errno_to_blk_status(PTR_ERR(rbio));
+		bio_endio(bio);
+		return;
 	}
 	rbio->operation = BTRFS_RBIO_WRITE;
 	rbio_add_bio(rbio, bio);
@@ -1674,31 +1674,24 @@ void raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc)
 	 * Don't plug on full rbios, just get them out the door
 	 * as quickly as we can
 	 */
-	if (rbio_is_full(rbio))
-		goto queue_rbio;
-
-	cb = blk_check_plugged(raid_unplug, fs_info, sizeof(*plug));
-	if (cb) {
-		plug = container_of(cb, struct btrfs_plug_cb, cb);
-		if (!plug->info) {
-			plug->info = fs_info;
-			INIT_LIST_HEAD(&plug->rbio_list);
+	if (!rbio_is_full(rbio)) {
+		cb = blk_check_plugged(raid_unplug, fs_info, sizeof(*plug));
+		if (cb) {
+			plug = container_of(cb, struct btrfs_plug_cb, cb);
+			if (!plug->info) {
+				plug->info = fs_info;
+				INIT_LIST_HEAD(&plug->rbio_list);
+			}
+			list_add_tail(&rbio->plug_list, &plug->rbio_list);
+			return;
 		}
-		list_add_tail(&rbio->plug_list, &plug->rbio_list);
-		return;
 	}
-queue_rbio:
+
 	/*
 	 * Either we don't have any existing plug, or we're doing a full stripe,
-	 * can queue the rmw work now.
+	 * queue the rmw work now.
 	 */
 	start_async_work(rbio, rmw_rbio_work);
-
-	return;
-
-fail:
-	bio->bi_status = errno_to_blk_status(ret);
-	bio_endio(bio);
 }
 
 static int verify_one_sector(struct btrfs_raid_bio *rbio,
-- 
2.35.1

[PATCH 02/10] btrfs: cleanup rmw_rbio

[Christoph Hellwig]

Remove the write goto label by moving the data page allocation and data
read into the branch.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/raid56.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 2cd5b1d7983009..0483f70a4fed74 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2293,24 +2293,22 @@ static int rmw_rbio(struct btrfs_raid_bio *rbio)
 	 * Either full stripe write, or we have every data sector already
 	 * cached, can go to write path immediately.
 	 */
-	if (rbio_is_full(rbio) || !need_read_stripe_sectors(rbio))
-		goto write;
-
-	/*
-	 * Now we're doing sub-stripe write, also need all data stripes to do
-	 * the full RMW.
-	 */
-	ret = alloc_rbio_data_pages(rbio);
-	if (ret < 0)
-		return ret;
+	if (!rbio_is_full(rbio) && need_read_stripe_sectors(rbio)) {
+		/*
+		 * Now we're doing sub-stripe write, also need all data stripes
+		 * to do the full RMW.
+		 */
+		ret = alloc_rbio_data_pages(rbio);
+		if (ret < 0)
+			return ret;
 
-	index_rbio_pages(rbio);
+		index_rbio_pages(rbio);
 
-	ret = rmw_read_wait_recover(rbio);
-	if (ret < 0)
-		return ret;
+		ret = rmw_read_wait_recover(rbio);
+		if (ret < 0)
+			return ret;
+	}
 
-write:
 	/*
 	 * At this stage we're not allowed to add any new bios to the
 	 * bio list any more, anyone else that wants to change this stripe
-- 
2.35.1

[PATCH 03/10] btrfs: wait for I/O completion in submit_read_bios

[Christoph Hellwig]

In addition to setting up the end_io handler and subitting the bios in
submit_read_bios, also wait for them to be completed instead of waiting
for the completion manually in all three callers.

Rename submit_read_bios to submit_read_wait_bio_list to make it clear
it waits for the bios as well.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 0483f70a4fed74..e3fef81a4d96d3 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1490,7 +1490,7 @@ static void raid_wait_read_end_io(struct bio *bio)
 		wake_up(&rbio->io_wait);
 }
 
-static void submit_read_bios(struct btrfs_raid_bio *rbio,
+static void submit_read_wait_bio_list(struct btrfs_raid_bio *rbio,
 			     struct bio_list *bio_list)
 {
 	struct bio *bio;
@@ -1507,6 +1507,8 @@ static void submit_read_bios(struct btrfs_raid_bio *rbio,
 		}
 		submit_bio(bio);
 	}
+
+	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
 }
 
 static int rmw_assemble_read_bios(struct btrfs_raid_bio *rbio,
@@ -2009,8 +2011,7 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
 	if (ret < 0)
 		goto out;
 
-	submit_read_bios(rbio, &bio_list);
-	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
+	submit_read_wait_bio_list(rbio, &bio_list);
 
 	ret = recover_sectors(rbio);
 
@@ -2206,8 +2207,7 @@ static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
 	if (ret < 0)
 		goto out;
 
-	submit_read_bios(rbio, &bio_list);
-	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
+	submit_read_wait_bio_list(rbio, &bio_list);
 
 	/*
 	 * We may or may not have any corrupted sectors (including missing dev
@@ -2785,8 +2785,7 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
 	if (ret < 0)
 		goto cleanup;
 
-	submit_read_bios(rbio, &bio_list);
-	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
+	submit_read_wait_bio_list(rbio, &bio_list);
 
 	/* We may have some failures, recover the failed sectors first. */
 	ret = recover_scrub_rbio(rbio);
-- 
2.35.1

Re: [PATCH 03/10] btrfs: wait for I/O completion in submit_read_bios

[Qu Wenruo]

On 2023/1/11 14:23, Christoph Hellwig wrote:
> In addition to setting up the end_io handler and subitting the bios in
> submit_read_bios, also wait for them to be completed instead of waiting
> for the completion manually in all three callers.
> 
> Rename submit_read_bios to submit_read_wait_bio_list to make it clear
> it waits for the bios as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/raid56.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 0483f70a4fed74..e3fef81a4d96d3 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1490,7 +1490,7 @@ static void raid_wait_read_end_io(struct bio *bio)
>   		wake_up(&rbio->io_wait);
>   }
>   
> -static void submit_read_bios(struct btrfs_raid_bio *rbio,
> +static void submit_read_wait_bio_list(struct btrfs_raid_bio *rbio,
>   			     struct bio_list *bio_list)
>   {
>   	struct bio *bio;
> @@ -1507,6 +1507,8 @@ static void submit_read_bios(struct btrfs_raid_bio *rbio,
>   		}
>   		submit_bio(bio);
>   	}
> +
> +	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
>   }
>   
>   static int rmw_assemble_read_bios(struct btrfs_raid_bio *rbio,
> @@ -2009,8 +2011,7 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
>   	if (ret < 0)
>   		goto out;
>   
> -	submit_read_bios(rbio, &bio_list);
> -	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
> +	submit_read_wait_bio_list(rbio, &bio_list);
>   
>   	ret = recover_sectors(rbio);
>   
> @@ -2206,8 +2207,7 @@ static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
>   	if (ret < 0)
>   		goto out;
>   
> -	submit_read_bios(rbio, &bio_list);
> -	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
> +	submit_read_wait_bio_list(rbio, &bio_list);
>   
>   	/*
>   	 * We may or may not have any corrupted sectors (including missing dev
> @@ -2785,8 +2785,7 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   	if (ret < 0)
>   		goto cleanup;
>   
> -	submit_read_bios(rbio, &bio_list);
> -	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
> +	submit_read_wait_bio_list(rbio, &bio_list);
>   
>   	/* We may have some failures, recover the failed sectors first. */
>   	ret = recover_scrub_rbio(rbio);

[PATCH 04/10] btrfs: add a bio_list_put helper

[Christoph Hellwig]

Add a helper to put all bios in a list.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/raid56.c | 44 ++++++++++++++++----------------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index e3fef81a4d96d3..666d634f0ba2c1 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1183,6 +1183,14 @@ static void bio_get_trace_info(struct btrfs_raid_bio *rbio, struct bio *bio,
 	trace_info->stripe_nr = -1;
 }
 
+static inline void bio_list_put(struct bio_list *bio_list)
+{
+	struct bio *bio;
+
+	while ((bio = bio_list_pop(bio_list)))
+		bio_put(bio);
+}
+
 /* Generate PQ for one veritical stripe. */
 static void generate_pq_vertical(struct btrfs_raid_bio *rbio, int sectornr)
 {
@@ -1228,7 +1236,6 @@ static void generate_pq_vertical(struct btrfs_raid_bio *rbio, int sectornr)
 static int rmw_assemble_write_bios(struct btrfs_raid_bio *rbio,
 				   struct bio_list *bio_list)
 {
-	struct bio *bio;
 	/* The total sector number inside the full stripe. */
 	int total_sector_nr;
 	int sectornr;
@@ -1317,8 +1324,7 @@ static int rmw_assemble_write_bios(struct btrfs_raid_bio *rbio,
 
 	return 0;
 error:
-	while ((bio = bio_list_pop(bio_list)))
-		bio_put(bio);
+	bio_list_put(bio_list);
 	return -EIO;
 }
 
@@ -1514,7 +1520,6 @@ static void submit_read_wait_bio_list(struct btrfs_raid_bio *rbio,
 static int rmw_assemble_read_bios(struct btrfs_raid_bio *rbio,
 				  struct bio_list *bio_list)
 {
-	struct bio *bio;
 	int total_sector_nr;
 	int ret = 0;
 
@@ -1541,8 +1546,7 @@ static int rmw_assemble_read_bios(struct btrfs_raid_bio *rbio,
 	return 0;
 
 cleanup:
-	while ((bio = bio_list_pop(bio_list)))
-		bio_put(bio);
+	bio_list_put(bio_list);
 	return ret;
 }
 
@@ -1939,7 +1943,6 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
 static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
 				      struct bio_list *bio_list)
 {
-	struct bio *bio;
 	int total_sector_nr;
 	int ret = 0;
 
@@ -1981,16 +1984,13 @@ static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
 	}
 	return 0;
 error:
-	while ((bio = bio_list_pop(bio_list)))
-		bio_put(bio);
-
+	bio_list_put(bio_list);
 	return -EIO;
 }
 
 static int recover_rbio(struct btrfs_raid_bio *rbio)
 {
 	struct bio_list bio_list;
-	struct bio *bio;
 	int ret;
 
 	/*
@@ -2016,9 +2016,7 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
 	ret = recover_sectors(rbio);
 
 out:
-	while ((bio = bio_list_pop(&bio_list)))
-		bio_put(bio);
-
+	bio_list_put(&bio_list);
 	return ret;
 }
 
@@ -2191,7 +2189,6 @@ static void fill_data_csums(struct btrfs_raid_bio *rbio)
 static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
 {
 	struct bio_list bio_list;
-	struct bio *bio;
 	int ret;
 
 	bio_list_init(&bio_list);
@@ -2216,9 +2213,7 @@ static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
 	ret = recover_sectors(rbio);
 	return ret;
 out:
-	while ((bio = bio_list_pop(&bio_list)))
-		bio_put(bio);
-
+	bio_list_put(&bio_list);
 	return ret;
 }
 
@@ -2489,7 +2484,6 @@ static int finish_parity_scrub(struct btrfs_raid_bio *rbio, int need_check)
 	struct sector_ptr p_sector = { 0 };
 	struct sector_ptr q_sector = { 0 };
 	struct bio_list bio_list;
-	struct bio *bio;
 	int is_replace = 0;
 	int ret;
 
@@ -2620,8 +2614,7 @@ static int finish_parity_scrub(struct btrfs_raid_bio *rbio, int need_check)
 	return 0;
 
 cleanup:
-	while ((bio = bio_list_pop(&bio_list)))
-		bio_put(bio);
+	bio_list_put(&bio_list);
 	return ret;
 }
 
@@ -2719,7 +2712,6 @@ static int recover_scrub_rbio(struct btrfs_raid_bio *rbio)
 static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
 				    struct bio_list *bio_list)
 {
-	struct bio *bio;
 	int total_sector_nr;
 	int ret = 0;
 
@@ -2760,8 +2752,7 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
 	}
 	return 0;
 error:
-	while ((bio = bio_list_pop(bio_list)))
-		bio_put(bio);
+	bio_list_put(bio_list);
 	return ret;
 }
 
@@ -2771,7 +2762,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
 	struct bio_list bio_list;
 	int sector_nr;
 	int ret;
-	struct bio *bio;
 
 	bio_list_init(&bio_list);
 
@@ -2810,9 +2800,7 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
 	return ret;
 
 cleanup:
-	while ((bio = bio_list_pop(&bio_list)))
-		bio_put(bio);
-
+	bio_list_put(&bio_list);
 	return ret;
 }
 
-- 
2.35.1

Re: [PATCH 04/10] btrfs: add a bio_list_put helper

[Qu Wenruo]

On 2023/1/11 14:23, Christoph Hellwig wrote:
> Add a helper to put all bios in a list.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/raid56.c | 44 ++++++++++++++++----------------------------
>   1 file changed, 16 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index e3fef81a4d96d3..666d634f0ba2c1 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1183,6 +1183,14 @@ static void bio_get_trace_info(struct btrfs_raid_bio *rbio, struct bio *bio,
>   	trace_info->stripe_nr = -1;
>   }
>   
> +static inline void bio_list_put(struct bio_list *bio_list)
> +{
> +	struct bio *bio;
> +
> +	while ((bio = bio_list_pop(bio_list)))
> +		bio_put(bio);
> +}
> +
>   /* Generate PQ for one veritical stripe. */
>   static void generate_pq_vertical(struct btrfs_raid_bio *rbio, int sectornr)
>   {
> @@ -1228,7 +1236,6 @@ static void generate_pq_vertical(struct btrfs_raid_bio *rbio, int sectornr)
>   static int rmw_assemble_write_bios(struct btrfs_raid_bio *rbio,
>   				   struct bio_list *bio_list)
>   {
> -	struct bio *bio;
>   	/* The total sector number inside the full stripe. */
>   	int total_sector_nr;
>   	int sectornr;
> @@ -1317,8 +1324,7 @@ static int rmw_assemble_write_bios(struct btrfs_raid_bio *rbio,
>   
>   	return 0;
>   error:
> -	while ((bio = bio_list_pop(bio_list)))
> -		bio_put(bio);
> +	bio_list_put(bio_list);
>   	return -EIO;
>   }
>   
> @@ -1514,7 +1520,6 @@ static void submit_read_wait_bio_list(struct btrfs_raid_bio *rbio,
>   static int rmw_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   				  struct bio_list *bio_list)
>   {
> -	struct bio *bio;
>   	int total_sector_nr;
>   	int ret = 0;
>   
> @@ -1541,8 +1546,7 @@ static int rmw_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   	return 0;
>   
>   cleanup:
> -	while ((bio = bio_list_pop(bio_list)))
> -		bio_put(bio);
> +	bio_list_put(bio_list);
>   	return ret;
>   }
>   
> @@ -1939,7 +1943,6 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
>   static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   				      struct bio_list *bio_list)
>   {
> -	struct bio *bio;
>   	int total_sector_nr;
>   	int ret = 0;
>   
> @@ -1981,16 +1984,13 @@ static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   	}
>   	return 0;
>   error:
> -	while ((bio = bio_list_pop(bio_list)))
> -		bio_put(bio);
> -
> +	bio_list_put(bio_list);
>   	return -EIO;
>   }
>   
>   static int recover_rbio(struct btrfs_raid_bio *rbio)
>   {
>   	struct bio_list bio_list;
> -	struct bio *bio;
>   	int ret;
>   
>   	/*
> @@ -2016,9 +2016,7 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
>   	ret = recover_sectors(rbio);
>   
>   out:
> -	while ((bio = bio_list_pop(&bio_list)))
> -		bio_put(bio);
> -
> +	bio_list_put(&bio_list);
>   	return ret;
>   }
>   
> @@ -2191,7 +2189,6 @@ static void fill_data_csums(struct btrfs_raid_bio *rbio)
>   static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
>   {
>   	struct bio_list bio_list;
> -	struct bio *bio;
>   	int ret;
>   
>   	bio_list_init(&bio_list);
> @@ -2216,9 +2213,7 @@ static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
>   	ret = recover_sectors(rbio);
>   	return ret;
>   out:
> -	while ((bio = bio_list_pop(&bio_list)))
> -		bio_put(bio);
> -
> +	bio_list_put(&bio_list);
>   	return ret;
>   }
>   
> @@ -2489,7 +2484,6 @@ static int finish_parity_scrub(struct btrfs_raid_bio *rbio, int need_check)
>   	struct sector_ptr p_sector = { 0 };
>   	struct sector_ptr q_sector = { 0 };
>   	struct bio_list bio_list;
> -	struct bio *bio;
>   	int is_replace = 0;
>   	int ret;
>   
> @@ -2620,8 +2614,7 @@ static int finish_parity_scrub(struct btrfs_raid_bio *rbio, int need_check)
>   	return 0;
>   
>   cleanup:
> -	while ((bio = bio_list_pop(&bio_list)))
> -		bio_put(bio);
> +	bio_list_put(&bio_list);
>   	return ret;
>   }
>   
> @@ -2719,7 +2712,6 @@ static int recover_scrub_rbio(struct btrfs_raid_bio *rbio)
>   static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   				    struct bio_list *bio_list)
>   {
> -	struct bio *bio;
>   	int total_sector_nr;
>   	int ret = 0;
>   
> @@ -2760,8 +2752,7 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   	}
>   	return 0;
>   error:
> -	while ((bio = bio_list_pop(bio_list)))
> -		bio_put(bio);
> +	bio_list_put(bio_list);
>   	return ret;
>   }
>   
> @@ -2771,7 +2762,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   	struct bio_list bio_list;
>   	int sector_nr;
>   	int ret;
> -	struct bio *bio;
>   
>   	bio_list_init(&bio_list);
>   
> @@ -2810,9 +2800,7 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   	return ret;
>   
>   cleanup:
> -	while ((bio = bio_list_pop(&bio_list)))
> -		bio_put(bio);
> -
> +	bio_list_put(&bio_list);
>   	return ret;
>   }
>   

Re: [PATCH 04/10] btrfs: add a bio_list_put helper

[Johannes Thumshirn]

On 11.01.23 07:24, Christoph Hellwig wrote:
> +static inline void bio_list_put(struct bio_list *bio_list)
> +{
> +	struct bio *bio;
> +
> +	while ((bio = bio_list_pop(bio_list)))
> +		bio_put(bio);
> +}
> +

Shouldn't that be lifted into bio.h? At least 
drivers/target/target_core_iblock.c would benefit from it as well.

Re: [PATCH 04/10] btrfs: add a bio_list_put helper

[Christoph Hellwig]

On Wed, Jan 11, 2023 at 11:12:21AM +0000, Johannes Thumshirn wrote:
> On 11.01.23 07:24, Christoph Hellwig wrote:
> > +static inline void bio_list_put(struct bio_list *bio_list)
> > +{
> > +	struct bio *bio;
> > +
> > +	while ((bio = bio_list_pop(bio_list)))
> > +		bio_put(bio);
> > +}
> > +
> 
> Shouldn't that be lifted into bio.h? At least 
> drivers/target/target_core_iblock.c would benefit from it as well.

That is in fact the only other two callers, out of which one is bogus
as we don't even need the list there (Write Same just has a single
block payload).  So for now I'd prefer to not move it to common
code.

Re: [PATCH 04/10] btrfs: add a bio_list_put helper

[Johannes Thumshirn]

On 12.01.23 09:10, Christoph Hellwig wrote:
> On Wed, Jan 11, 2023 at 11:12:21AM +0000, Johannes Thumshirn wrote:
>> On 11.01.23 07:24, Christoph Hellwig wrote:
>>> +static inline void bio_list_put(struct bio_list *bio_list)
>>> +{
>>> +	struct bio *bio;
>>> +
>>> +	while ((bio = bio_list_pop(bio_list)))
>>> +		bio_put(bio);
>>> +}
>>> +
>>
>> Shouldn't that be lifted into bio.h? At least 
>> drivers/target/target_core_iblock.c would benefit from it as well.
> 
> That is in fact the only other two callers, out of which one is bogus
> as we don't even need the list there (Write Same just has a single
> block payload).  So for now I'd prefer to not move it to common
> code.
> 

OK then let's not go down that rabbit hole.

[PATCH 05/10] btrfs: fold recover_assemble_read_bios into recover_rbio

[Christoph Hellwig]

There is very little extra code in recover_rbio, and a large part of it
is the superflous extra cleanup of the bio list.  Merge the two
functions, and only clean up the bio list after it has been added to
but before it has been emptied again by submit_read_wait_bio_list.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/raid56.c | 61 ++++++++++++++++-------------------------------
 1 file changed, 21 insertions(+), 40 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 666d634f0ba2c1..4e983ca8cd532c 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1940,13 +1940,25 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
 	return ret;
 }
 
-static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
-				      struct bio_list *bio_list)
+static int recover_rbio(struct btrfs_raid_bio *rbio)
 {
+	struct bio_list bio_list = BIO_EMPTY_LIST;
 	int total_sector_nr;
 	int ret = 0;
 
-	ASSERT(bio_list_size(bio_list) == 0);
+	/*
+	 * Either we're doing recover for a read failure or degraded write,
+	 * caller should have set error bitmap correctly.
+	 */
+	ASSERT(bitmap_weight(rbio->error_bitmap, rbio->nr_sectors));
+
+	/* For recovery, we need to read all sectors including P/Q. */
+	ret = alloc_rbio_pages(rbio);
+	if (ret < 0)
+		return ret;
+
+	index_rbio_pages(rbio);
+
 	/*
 	 * Read everything that hasn't failed. However this time we will
 	 * not trust any cached sector.
@@ -1977,47 +1989,16 @@ static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
 		}
 
 		sector = rbio_stripe_sector(rbio, stripe, sectornr);
-		ret = rbio_add_io_sector(rbio, bio_list, sector, stripe,
+		ret = rbio_add_io_sector(rbio, &bio_list, sector, stripe,
 					 sectornr, REQ_OP_READ);
-		if (ret < 0)
-			goto error;
+		if (ret < 0) {
+			bio_list_put(&bio_list);
+			return ret;
+		}
 	}
-	return 0;
-error:
-	bio_list_put(bio_list);
-	return -EIO;
-}
-
-static int recover_rbio(struct btrfs_raid_bio *rbio)
-{
-	struct bio_list bio_list;
-	int ret;
-
-	/*
-	 * Either we're doing recover for a read failure or degraded write,
-	 * caller should have set error bitmap correctly.
-	 */
-	ASSERT(bitmap_weight(rbio->error_bitmap, rbio->nr_sectors));
-	bio_list_init(&bio_list);
-
-	/* For recovery, we need to read all sectors including P/Q. */
-	ret = alloc_rbio_pages(rbio);
-	if (ret < 0)
-		goto out;
-
-	index_rbio_pages(rbio);
-
-	ret = recover_assemble_read_bios(rbio, &bio_list);
-	if (ret < 0)
-		goto out;
 
 	submit_read_wait_bio_list(rbio, &bio_list);
-
-	ret = recover_sectors(rbio);
-
-out:
-	bio_list_put(&bio_list);
-	return ret;
+	return recover_sectors(rbio);
 }
 
 static void recover_rbio_work(struct work_struct *work)
-- 
2.35.1

Re: [PATCH 05/10] btrfs: fold recover_assemble_read_bios into recover_rbio

[Qu Wenruo]

On 2023/1/11 14:23, Christoph Hellwig wrote:
> There is very little extra code in recover_rbio, and a large part of it
> is the superflous extra cleanup of the bio list.  Merge the two
> functions, and only clean up the bio list after it has been added to
> but before it has been emptied again by submit_read_wait_bio_list.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/raid56.c | 61 ++++++++++++++++-------------------------------
>   1 file changed, 21 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 666d634f0ba2c1..4e983ca8cd532c 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1940,13 +1940,25 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
>   	return ret;
>   }
>   
> -static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
> -				      struct bio_list *bio_list)
> +static int recover_rbio(struct btrfs_raid_bio *rbio)
>   {
> +	struct bio_list bio_list = BIO_EMPTY_LIST;
>   	int total_sector_nr;
>   	int ret = 0;
>   
> -	ASSERT(bio_list_size(bio_list) == 0);
> +	/*
> +	 * Either we're doing recover for a read failure or degraded write,
> +	 * caller should have set error bitmap correctly.
> +	 */
> +	ASSERT(bitmap_weight(rbio->error_bitmap, rbio->nr_sectors));
> +
> +	/* For recovery, we need to read all sectors including P/Q. */
> +	ret = alloc_rbio_pages(rbio);
> +	if (ret < 0)
> +		return ret;
> +
> +	index_rbio_pages(rbio);
> +
>   	/*
>   	 * Read everything that hasn't failed. However this time we will
>   	 * not trust any cached sector.
> @@ -1977,47 +1989,16 @@ static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   		}
>   
>   		sector = rbio_stripe_sector(rbio, stripe, sectornr);
> -		ret = rbio_add_io_sector(rbio, bio_list, sector, stripe,
> +		ret = rbio_add_io_sector(rbio, &bio_list, sector, stripe,
>   					 sectornr, REQ_OP_READ);
> -		if (ret < 0)
> -			goto error;
> +		if (ret < 0) {
> +			bio_list_put(&bio_list);
> +			return ret;
> +		}
>   	}
> -	return 0;
> -error:
> -	bio_list_put(bio_list);
> -	return -EIO;
> -}
> -
> -static int recover_rbio(struct btrfs_raid_bio *rbio)
> -{
> -	struct bio_list bio_list;
> -	int ret;
> -
> -	/*
> -	 * Either we're doing recover for a read failure or degraded write,
> -	 * caller should have set error bitmap correctly.
> -	 */
> -	ASSERT(bitmap_weight(rbio->error_bitmap, rbio->nr_sectors));
> -	bio_list_init(&bio_list);
> -
> -	/* For recovery, we need to read all sectors including P/Q. */
> -	ret = alloc_rbio_pages(rbio);
> -	if (ret < 0)
> -		goto out;
> -
> -	index_rbio_pages(rbio);
> -
> -	ret = recover_assemble_read_bios(rbio, &bio_list);
> -	if (ret < 0)
> -		goto out;
>   
>   	submit_read_wait_bio_list(rbio, &bio_list);
> -
> -	ret = recover_sectors(rbio);
> -
> -out:
> -	bio_list_put(&bio_list);
> -	return ret;
> +	return recover_sectors(rbio);
>   }
>   
>   static void recover_rbio_work(struct work_struct *work)

[PATCH 06/10] btrfs: fold rmw_read_wait_recover into rmw_read_bios

[Christoph Hellwig]

There is very little extra code in rmw_read_bios, and a large part of it
is the superflous extra cleanup of the bio list.  Merge the two
functions, and only clean up the bio list after it has been added to
but before it has been emptied again by submit_read_wait_bio_list.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/raid56.c | 70 ++++++++++++++++-------------------------------
 1 file changed, 24 insertions(+), 46 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 4e983ca8cd532c..88404a6b0b98e7 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1517,39 +1517,6 @@ static void submit_read_wait_bio_list(struct btrfs_raid_bio *rbio,
 	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
 }
 
-static int rmw_assemble_read_bios(struct btrfs_raid_bio *rbio,
-				  struct bio_list *bio_list)
-{
-	int total_sector_nr;
-	int ret = 0;
-
-	ASSERT(bio_list_size(bio_list) == 0);
-
-	/*
-	 * Build a list of bios to read all sectors (including data and P/Q).
-	 *
-	 * This behaviro is to compensate the later csum verification and
-	 * recovery.
-	 */
-	for (total_sector_nr = 0; total_sector_nr < rbio->nr_sectors;
-	     total_sector_nr++) {
-		struct sector_ptr *sector;
-		int stripe = total_sector_nr / rbio->stripe_nsectors;
-		int sectornr = total_sector_nr % rbio->stripe_nsectors;
-
-		sector = rbio_stripe_sector(rbio, stripe, sectornr);
-		ret = rbio_add_io_sector(rbio, bio_list, sector,
-			       stripe, sectornr, REQ_OP_READ);
-		if (ret)
-			goto cleanup;
-	}
-	return 0;
-
-cleanup:
-	bio_list_put(bio_list);
-	return ret;
-}
-
 static int alloc_rbio_data_pages(struct btrfs_raid_bio *rbio)
 {
 	const int data_pages = rbio->nr_data * rbio->stripe_npages;
@@ -2169,10 +2136,9 @@ static void fill_data_csums(struct btrfs_raid_bio *rbio)
 
 static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
 {
-	struct bio_list bio_list;
-	int ret;
-
-	bio_list_init(&bio_list);
+	struct bio_list bio_list = BIO_EMPTY_LIST;
+	int total_sector_nr;
+	int ret = 0;
 
 	/*
 	 * Fill the data csums we need for data verification.  We need to fill
@@ -2181,21 +2147,33 @@ static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
 	 */
 	fill_data_csums(rbio);
 
-	ret = rmw_assemble_read_bios(rbio, &bio_list);
-	if (ret < 0)
-		goto out;
+	/*
+	 * Build a list of bios to read all sectors (including data and P/Q).
+	 *
+	 * This behaviro is to compensate the later csum verification and
+	 * recovery.
+	 */
+	for (total_sector_nr = 0; total_sector_nr < rbio->nr_sectors;
+	     total_sector_nr++) {
+		struct sector_ptr *sector;
+		int stripe = total_sector_nr / rbio->stripe_nsectors;
+		int sectornr = total_sector_nr % rbio->stripe_nsectors;
 
-	submit_read_wait_bio_list(rbio, &bio_list);
+		sector = rbio_stripe_sector(rbio, stripe, sectornr);
+		ret = rbio_add_io_sector(rbio, &bio_list, sector,
+			       stripe, sectornr, REQ_OP_READ);
+		if (ret) {
+			bio_list_put(&bio_list);
+			return ret;
+		}
+	}
 
 	/*
 	 * We may or may not have any corrupted sectors (including missing dev
 	 * and csum mismatch), just let recover_sectors() to handle them all.
 	 */
-	ret = recover_sectors(rbio);
-	return ret;
-out:
-	bio_list_put(&bio_list);
-	return ret;
+	submit_read_wait_bio_list(rbio, &bio_list);
+	return recover_sectors(rbio);
 }
 
 static void raid_wait_write_end_io(struct bio *bio)
-- 
2.35.1

Re: [PATCH 06/10] btrfs: fold rmw_read_wait_recover into rmw_read_bios

[Qu Wenruo]

On 2023/1/11 14:23, Christoph Hellwig wrote:
> There is very little extra code in rmw_read_bios, and a large part of it
> is the superflous extra cleanup of the bio list.  Merge the two
> functions, and only clean up the bio list after it has been added to
> but before it has been emptied again by submit_read_wait_bio_list.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/raid56.c | 70 ++++++++++++++++-------------------------------
>   1 file changed, 24 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 4e983ca8cd532c..88404a6b0b98e7 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1517,39 +1517,6 @@ static void submit_read_wait_bio_list(struct btrfs_raid_bio *rbio,
>   	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
>   }
>   
> -static int rmw_assemble_read_bios(struct btrfs_raid_bio *rbio,
> -				  struct bio_list *bio_list)
> -{
> -	int total_sector_nr;
> -	int ret = 0;
> -
> -	ASSERT(bio_list_size(bio_list) == 0);
> -
> -	/*
> -	 * Build a list of bios to read all sectors (including data and P/Q).
> -	 *
> -	 * This behaviro is to compensate the later csum verification and
> -	 * recovery.
> -	 */
> -	for (total_sector_nr = 0; total_sector_nr < rbio->nr_sectors;
> -	     total_sector_nr++) {
> -		struct sector_ptr *sector;
> -		int stripe = total_sector_nr / rbio->stripe_nsectors;
> -		int sectornr = total_sector_nr % rbio->stripe_nsectors;
> -
> -		sector = rbio_stripe_sector(rbio, stripe, sectornr);
> -		ret = rbio_add_io_sector(rbio, bio_list, sector,
> -			       stripe, sectornr, REQ_OP_READ);
> -		if (ret)
> -			goto cleanup;
> -	}
> -	return 0;
> -
> -cleanup:
> -	bio_list_put(bio_list);
> -	return ret;
> -}
> -
>   static int alloc_rbio_data_pages(struct btrfs_raid_bio *rbio)
>   {
>   	const int data_pages = rbio->nr_data * rbio->stripe_npages;
> @@ -2169,10 +2136,9 @@ static void fill_data_csums(struct btrfs_raid_bio *rbio)
>   
>   static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
>   {
> -	struct bio_list bio_list;
> -	int ret;
> -
> -	bio_list_init(&bio_list);
> +	struct bio_list bio_list = BIO_EMPTY_LIST;
> +	int total_sector_nr;
> +	int ret = 0;
>   
>   	/*
>   	 * Fill the data csums we need for data verification.  We need to fill
> @@ -2181,21 +2147,33 @@ static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
>   	 */
>   	fill_data_csums(rbio);
>   
> -	ret = rmw_assemble_read_bios(rbio, &bio_list);
> -	if (ret < 0)
> -		goto out;
> +	/*
> +	 * Build a list of bios to read all sectors (including data and P/Q).
> +	 *
> +	 * This behaviro is to compensate the later csum verification and
> +	 * recovery.
> +	 */
> +	for (total_sector_nr = 0; total_sector_nr < rbio->nr_sectors;
> +	     total_sector_nr++) {
> +		struct sector_ptr *sector;
> +		int stripe = total_sector_nr / rbio->stripe_nsectors;
> +		int sectornr = total_sector_nr % rbio->stripe_nsectors;
>   
> -	submit_read_wait_bio_list(rbio, &bio_list);
> +		sector = rbio_stripe_sector(rbio, stripe, sectornr);
> +		ret = rbio_add_io_sector(rbio, &bio_list, sector,
> +			       stripe, sectornr, REQ_OP_READ);
> +		if (ret) {
> +			bio_list_put(&bio_list);
> +			return ret;
> +		}
> +	}
>   
>   	/*
>   	 * We may or may not have any corrupted sectors (including missing dev
>   	 * and csum mismatch), just let recover_sectors() to handle them all.
>   	 */
> -	ret = recover_sectors(rbio);
> -	return ret;
> -out:
> -	bio_list_put(&bio_list);
> -	return ret;
> +	submit_read_wait_bio_list(rbio, &bio_list);
> +	return recover_sectors(rbio);
>   }
>   
>   static void raid_wait_write_end_io(struct bio *bio)

[PATCH 07/10] btrfs: submit the read bios from scrub_assemble_read_bios

[Christoph Hellwig]

Instead of filling in a bio_list and submitting the bios in the only
caller, do that in scrub_assemble_read_bios.  This removes the
need to pass the bio_list, and also makes it clear that the extra
bio_list cleanup in the caller is entirely pointless.  Rename the
function to scrub_read_bios to make it clear that the bios are not
only assembled.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/raid56.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 88404a6b0b98e7..374c3873169b3f 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2668,14 +2668,12 @@ static int recover_scrub_rbio(struct btrfs_raid_bio *rbio)
 	return ret;
 }
 
-static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
-				    struct bio_list *bio_list)
+static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio)
 {
+	struct bio_list bio_list = BIO_EMPTY_LIST;
 	int total_sector_nr;
 	int ret = 0;
 
-	ASSERT(bio_list_size(bio_list) == 0);
-
 	/* Build a list of bios to read all the missing parts. */
 	for (total_sector_nr = 0; total_sector_nr < rbio->nr_sectors;
 	     total_sector_nr++) {
@@ -2704,42 +2702,38 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
 		if (sector->uptodate)
 			continue;
 
-		ret = rbio_add_io_sector(rbio, bio_list, sector, stripe,
+		ret = rbio_add_io_sector(rbio, &bio_list, sector, stripe,
 					 sectornr, REQ_OP_READ);
-		if (ret)
-			goto error;
+		if (ret) {
+			bio_list_put(&bio_list);
+			return ret;
+		}
 	}
+
+	submit_read_wait_bio_list(rbio, &bio_list);
 	return 0;
-error:
-	bio_list_put(bio_list);
-	return ret;
 }
 
 static int scrub_rbio(struct btrfs_raid_bio *rbio)
 {
 	bool need_check = false;
-	struct bio_list bio_list;
 	int sector_nr;
 	int ret;
 
-	bio_list_init(&bio_list);
-
 	ret = alloc_rbio_essential_pages(rbio);
 	if (ret)
-		goto cleanup;
+		return ret;
 
 	bitmap_clear(rbio->error_bitmap, 0, rbio->nr_sectors);
 
-	ret = scrub_assemble_read_bios(rbio, &bio_list);
+	ret = scrub_assemble_read_bios(rbio);
 	if (ret < 0)
-		goto cleanup;
-
-	submit_read_wait_bio_list(rbio, &bio_list);
+		return ret;
 
 	/* We may have some failures, recover the failed sectors first. */
 	ret = recover_scrub_rbio(rbio);
 	if (ret < 0)
-		goto cleanup;
+		return ret;
 
 	/*
 	 * We have every sector properly prepared. Can finish the scrub
@@ -2757,10 +2751,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
 		}
 	}
 	return ret;
-
-cleanup:
-	bio_list_put(&bio_list);
-	return ret;
 }
 
 static void scrub_rbio_work_locked(struct work_struct *work)
-- 
2.35.1

Re: [PATCH 07/10] btrfs: submit the read bios from scrub_assemble_read_bios

[Qu Wenruo]

On 2023/1/11 14:23, Christoph Hellwig wrote:
> Instead of filling in a bio_list and submitting the bios in the only
> caller, do that in scrub_assemble_read_bios.  This removes the
> need to pass the bio_list, and also makes it clear that the extra
> bio_list cleanup in the caller is entirely pointless.  Rename the
> function to scrub_read_bios to make it clear that the bios are not
> only assembled.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Originally the idea of passing bio_list around is reuse the function to 
submit and wait.

But your cleanup has shown it can be smaller and simpler even without 
reusing the same submit and wait function.

Thanks,
Qu
> ---
>   fs/btrfs/raid56.c | 36 +++++++++++++-----------------------
>   1 file changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 88404a6b0b98e7..374c3873169b3f 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2668,14 +2668,12 @@ static int recover_scrub_rbio(struct btrfs_raid_bio *rbio)
>   	return ret;
>   }
>   
> -static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
> -				    struct bio_list *bio_list)
> +static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio)
>   {
> +	struct bio_list bio_list = BIO_EMPTY_LIST;
>   	int total_sector_nr;
>   	int ret = 0;
>   
> -	ASSERT(bio_list_size(bio_list) == 0);
> -
>   	/* Build a list of bios to read all the missing parts. */
>   	for (total_sector_nr = 0; total_sector_nr < rbio->nr_sectors;
>   	     total_sector_nr++) {
> @@ -2704,42 +2702,38 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   		if (sector->uptodate)
>   			continue;
>   
> -		ret = rbio_add_io_sector(rbio, bio_list, sector, stripe,
> +		ret = rbio_add_io_sector(rbio, &bio_list, sector, stripe,
>   					 sectornr, REQ_OP_READ);
> -		if (ret)
> -			goto error;
> +		if (ret) {
> +			bio_list_put(&bio_list);
> +			return ret;
> +		}
>   	}
> +
> +	submit_read_wait_bio_list(rbio, &bio_list);
>   	return 0;
> -error:
> -	bio_list_put(bio_list);
> -	return ret;
>   }
>   
>   static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   {
>   	bool need_check = false;
> -	struct bio_list bio_list;
>   	int sector_nr;
>   	int ret;
>   
> -	bio_list_init(&bio_list);
> -
>   	ret = alloc_rbio_essential_pages(rbio);
>   	if (ret)
> -		goto cleanup;
> +		return ret;
>   
>   	bitmap_clear(rbio->error_bitmap, 0, rbio->nr_sectors);
>   
> -	ret = scrub_assemble_read_bios(rbio, &bio_list);
> +	ret = scrub_assemble_read_bios(rbio);
>   	if (ret < 0)
> -		goto cleanup;
> -
> -	submit_read_wait_bio_list(rbio, &bio_list);
> +		return ret;
>   
>   	/* We may have some failures, recover the failed sectors first. */
>   	ret = recover_scrub_rbio(rbio);
>   	if (ret < 0)
> -		goto cleanup;
> +		return ret;
>   
>   	/*
>   	 * We have every sector properly prepared. Can finish the scrub
> @@ -2757,10 +2751,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   		}
>   	}
>   	return ret;
> -
> -cleanup:
> -	bio_list_put(&bio_list);
> -	return ret;
>   }
>   
>   static void scrub_rbio_work_locked(struct work_struct *work)

[PATCH 08/10] btrfs: call rbio_orig_end_io from rmw_rbio

[Christoph Hellwig]

Both callers of rmv_rbio call rbio_orig_end_io right after it, so
move the call into the shared function.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 374c3873169b3f..a9947477daf26d 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2229,7 +2229,7 @@ static bool need_read_stripe_sectors(struct btrfs_raid_bio *rbio)
 	return false;
 }
 
-static int rmw_rbio(struct btrfs_raid_bio *rbio)
+static void rmw_rbio(struct btrfs_raid_bio *rbio)
 {
 	struct bio_list bio_list;
 	int sectornr;
@@ -2241,7 +2241,7 @@ static int rmw_rbio(struct btrfs_raid_bio *rbio)
 	 */
 	ret = alloc_rbio_parity_pages(rbio);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	/*
 	 * Either full stripe write, or we have every data sector already
@@ -2254,13 +2254,13 @@ static int rmw_rbio(struct btrfs_raid_bio *rbio)
 		 */
 		ret = alloc_rbio_data_pages(rbio);
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		index_rbio_pages(rbio);
 
 		ret = rmw_read_wait_recover(rbio);
 		if (ret < 0)
-			return ret;
+			goto out;
 	}
 
 	/*
@@ -2293,7 +2293,7 @@ static int rmw_rbio(struct btrfs_raid_bio *rbio)
 	bio_list_init(&bio_list);
 	ret = rmw_assemble_write_bios(rbio, &bio_list);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	/* We should have at least one bio assembled. */
 	ASSERT(bio_list_size(&bio_list));
@@ -2310,32 +2310,22 @@ static int rmw_rbio(struct btrfs_raid_bio *rbio)
 			break;
 		}
 	}
-	return ret;
+out:
+	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
 }
 
 static void rmw_rbio_work(struct work_struct *work)
 {
 	struct btrfs_raid_bio *rbio;
-	int ret;
 
 	rbio = container_of(work, struct btrfs_raid_bio, work);
-
-	ret = lock_stripe_add(rbio);
-	if (ret == 0) {
-		ret = rmw_rbio(rbio);
-		rbio_orig_end_io(rbio, errno_to_blk_status(ret));
-	}
+	if (lock_stripe_add(rbio) == 0)
+		rmw_rbio(rbio);
 }
 
 static void rmw_rbio_work_locked(struct work_struct *work)
 {
-	struct btrfs_raid_bio *rbio;
-	int ret;
-
-	rbio = container_of(work, struct btrfs_raid_bio, work);
-
-	ret = rmw_rbio(rbio);
-	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
+	rmw_rbio(container_of(work, struct btrfs_raid_bio, work));
 }
 
 /*
-- 
2.35.1

Re: [PATCH 08/10] btrfs: call rbio_orig_end_io from rmw_rbio

[Qu Wenruo]

On 2023/1/11 14:23, Christoph Hellwig wrote:
> Both callers of rmv_rbio call rbio_orig_end_io right after it, so
> move the call into the shared function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/raid56.c | 30 ++++++++++--------------------
>   1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 374c3873169b3f..a9947477daf26d 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2229,7 +2229,7 @@ static bool need_read_stripe_sectors(struct btrfs_raid_bio *rbio)
>   	return false;
>   }
>   
> -static int rmw_rbio(struct btrfs_raid_bio *rbio)
> +static void rmw_rbio(struct btrfs_raid_bio *rbio)
>   {
>   	struct bio_list bio_list;
>   	int sectornr;
> @@ -2241,7 +2241,7 @@ static int rmw_rbio(struct btrfs_raid_bio *rbio)
>   	 */
>   	ret = alloc_rbio_parity_pages(rbio);
>   	if (ret < 0)
> -		return ret;
> +		goto out;
>   
>   	/*
>   	 * Either full stripe write, or we have every data sector already
> @@ -2254,13 +2254,13 @@ static int rmw_rbio(struct btrfs_raid_bio *rbio)
>   		 */
>   		ret = alloc_rbio_data_pages(rbio);
>   		if (ret < 0)
> -			return ret;
> +			goto out;
>   
>   		index_rbio_pages(rbio);
>   
>   		ret = rmw_read_wait_recover(rbio);
>   		if (ret < 0)
> -			return ret;
> +			goto out;
>   	}
>   
>   	/*
> @@ -2293,7 +2293,7 @@ static int rmw_rbio(struct btrfs_raid_bio *rbio)
>   	bio_list_init(&bio_list);
>   	ret = rmw_assemble_write_bios(rbio, &bio_list);
>   	if (ret < 0)
> -		return ret;
> +		goto out;
>   
>   	/* We should have at least one bio assembled. */
>   	ASSERT(bio_list_size(&bio_list));
> @@ -2310,32 +2310,22 @@ static int rmw_rbio(struct btrfs_raid_bio *rbio)
>   			break;
>   		}
>   	}
> -	return ret;
> +out:
> +	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
>   }
>   
>   static void rmw_rbio_work(struct work_struct *work)
>   {
>   	struct btrfs_raid_bio *rbio;
> -	int ret;
>   
>   	rbio = container_of(work, struct btrfs_raid_bio, work);
> -
> -	ret = lock_stripe_add(rbio);
> -	if (ret == 0) {
> -		ret = rmw_rbio(rbio);
> -		rbio_orig_end_io(rbio, errno_to_blk_status(ret));
> -	}
> +	if (lock_stripe_add(rbio) == 0)
> +		rmw_rbio(rbio);
>   }
>   
>   static void rmw_rbio_work_locked(struct work_struct *work)
>   {
> -	struct btrfs_raid_bio *rbio;
> -	int ret;
> -
> -	rbio = container_of(work, struct btrfs_raid_bio, work);
> -
> -	ret = rmw_rbio(rbio);
> -	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
> +	rmw_rbio(container_of(work, struct btrfs_raid_bio, work));
>   }
>   
>   /*

[PATCH 09/10] btrfs: call rbio_orig_end_io from recover_rbio

[Christoph Hellwig]

Both callers of recover_rbio call rbio_orig_end_io right after it, so
move the call into the shared function.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/raid56.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index a9947477daf26d..c007992bf4426c 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1907,7 +1907,7 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
 	return ret;
 }
 
-static int recover_rbio(struct btrfs_raid_bio *rbio)
+static void recover_rbio(struct btrfs_raid_bio *rbio)
 {
 	struct bio_list bio_list = BIO_EMPTY_LIST;
 	int total_sector_nr;
@@ -1922,7 +1922,7 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
 	/* For recovery, we need to read all sectors including P/Q. */
 	ret = alloc_rbio_pages(rbio);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	index_rbio_pages(rbio);
 
@@ -1960,37 +1960,28 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
 					 sectornr, REQ_OP_READ);
 		if (ret < 0) {
 			bio_list_put(&bio_list);
-			return ret;
+			goto out;
 		}
 	}
 
 	submit_read_wait_bio_list(rbio, &bio_list);
-	return recover_sectors(rbio);
+	ret = recover_sectors(rbio);
+out:
+	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
 }
 
 static void recover_rbio_work(struct work_struct *work)
 {
 	struct btrfs_raid_bio *rbio;
-	int ret;
 
 	rbio = container_of(work, struct btrfs_raid_bio, work);
-
-	ret = lock_stripe_add(rbio);
-	if (ret == 0) {
-		ret = recover_rbio(rbio);
-		rbio_orig_end_io(rbio, errno_to_blk_status(ret));
-	}
+	if (!lock_stripe_add(rbio))
+		recover_rbio(rbio);
 }
 
 static void recover_rbio_work_locked(struct work_struct *work)
 {
-	struct btrfs_raid_bio *rbio;
-	int ret;
-
-	rbio = container_of(work, struct btrfs_raid_bio, work);
-
-	ret = recover_rbio(rbio);
-	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
+	recover_rbio(container_of(work, struct btrfs_raid_bio, work));
 }
 
 static void set_rbio_raid6_extra_error(struct btrfs_raid_bio *rbio, int mirror_num)
-- 
2.35.1

Re: [PATCH 09/10] btrfs: call rbio_orig_end_io from recover_rbio

[Qu Wenruo]

On 2023/1/11 14:23, Christoph Hellwig wrote:
> Both callers of recover_rbio call rbio_orig_end_io right after it, so
> move the call into the shared function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/raid56.c | 27 +++++++++------------------
>   1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index a9947477daf26d..c007992bf4426c 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1907,7 +1907,7 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
>   	return ret;
>   }
>   
> -static int recover_rbio(struct btrfs_raid_bio *rbio)
> +static void recover_rbio(struct btrfs_raid_bio *rbio)
>   {
>   	struct bio_list bio_list = BIO_EMPTY_LIST;
>   	int total_sector_nr;
> @@ -1922,7 +1922,7 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
>   	/* For recovery, we need to read all sectors including P/Q. */
>   	ret = alloc_rbio_pages(rbio);
>   	if (ret < 0)
> -		return ret;
> +		goto out;
>   
>   	index_rbio_pages(rbio);
>   
> @@ -1960,37 +1960,28 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
>   					 sectornr, REQ_OP_READ);
>   		if (ret < 0) {
>   			bio_list_put(&bio_list);
> -			return ret;
> +			goto out;
>   		}
>   	}
>   
>   	submit_read_wait_bio_list(rbio, &bio_list);
> -	return recover_sectors(rbio);
> +	ret = recover_sectors(rbio);
> +out:
> +	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
>   }
>   
>   static void recover_rbio_work(struct work_struct *work)
>   {
>   	struct btrfs_raid_bio *rbio;
> -	int ret;
>   
>   	rbio = container_of(work, struct btrfs_raid_bio, work);
> -
> -	ret = lock_stripe_add(rbio);
> -	if (ret == 0) {
> -		ret = recover_rbio(rbio);
> -		rbio_orig_end_io(rbio, errno_to_blk_status(ret));
> -	}
> +	if (!lock_stripe_add(rbio))
> +		recover_rbio(rbio);
>   }
>   
>   static void recover_rbio_work_locked(struct work_struct *work)
>   {
> -	struct btrfs_raid_bio *rbio;
> -	int ret;
> -
> -	rbio = container_of(work, struct btrfs_raid_bio, work);
> -
> -	ret = recover_rbio(rbio);
> -	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
> +	recover_rbio(container_of(work, struct btrfs_raid_bio, work));
>   }
>   
>   static void set_rbio_raid6_extra_error(struct btrfs_raid_bio *rbio, int mirror_num)

[PATCH 10/10] btrfs: call rbio_orig_end_io from scrub_rbio

[Christoph Hellwig]

The only caller of scrub_rbio calls rbio_orig_end_io right after it,
move it into scrub_rbio to match the other work item helpers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/raid56.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index c007992bf4426c..d8dd25a8155a52 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2695,7 +2695,7 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio)
 	return 0;
 }
 
-static int scrub_rbio(struct btrfs_raid_bio *rbio)
+static void scrub_rbio(struct btrfs_raid_bio *rbio)
 {
 	bool need_check = false;
 	int sector_nr;
@@ -2703,18 +2703,18 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
 
 	ret = alloc_rbio_essential_pages(rbio);
 	if (ret)
-		return ret;
+		goto out;
 
 	bitmap_clear(rbio->error_bitmap, 0, rbio->nr_sectors);
 
 	ret = scrub_assemble_read_bios(rbio);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	/* We may have some failures, recover the failed sectors first. */
 	ret = recover_scrub_rbio(rbio);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	/*
 	 * We have every sector properly prepared. Can finish the scrub
@@ -2731,17 +2731,13 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
 			break;
 		}
 	}
-	return ret;
+out:
+	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
 }
 
 static void scrub_rbio_work_locked(struct work_struct *work)
 {
-	struct btrfs_raid_bio *rbio;
-	int ret;
-
-	rbio = container_of(work, struct btrfs_raid_bio, work);
-	ret = scrub_rbio(rbio);
-	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
+	scrub_rbio(container_of(work, struct btrfs_raid_bio, work));
 }
 
 void raid56_parity_submit_scrub_rbio(struct btrfs_raid_bio *rbio)
-- 
2.35.1

Re: [PATCH 10/10] btrfs: call rbio_orig_end_io from scrub_rbio

[Qu Wenruo]

On 2023/1/11 14:23, Christoph Hellwig wrote:
> The only caller of scrub_rbio calls rbio_orig_end_io right after it,
> move it into scrub_rbio to match the other work item helpers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/raid56.c | 18 +++++++-----------
>   1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index c007992bf4426c..d8dd25a8155a52 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2695,7 +2695,7 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio)
>   	return 0;
>   }
>   
> -static int scrub_rbio(struct btrfs_raid_bio *rbio)
> +static void scrub_rbio(struct btrfs_raid_bio *rbio)
>   {
>   	bool need_check = false;
>   	int sector_nr;
> @@ -2703,18 +2703,18 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   
>   	ret = alloc_rbio_essential_pages(rbio);
>   	if (ret)
> -		return ret;
> +		goto out;
>   
>   	bitmap_clear(rbio->error_bitmap, 0, rbio->nr_sectors);
>   
>   	ret = scrub_assemble_read_bios(rbio);
>   	if (ret < 0)
> -		return ret;
> +		goto out;
>   
>   	/* We may have some failures, recover the failed sectors first. */
>   	ret = recover_scrub_rbio(rbio);
>   	if (ret < 0)
> -		return ret;
> +		goto out;
>   
>   	/*
>   	 * We have every sector properly prepared. Can finish the scrub
> @@ -2731,17 +2731,13 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   			break;
>   		}
>   	}
> -	return ret;
> +out:
> +	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
>   }
>   
>   static void scrub_rbio_work_locked(struct work_struct *work)
>   {
> -	struct btrfs_raid_bio *rbio;
> -	int ret;
> -
> -	rbio = container_of(work, struct btrfs_raid_bio, work);
> -	ret = scrub_rbio(rbio);
> -	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
> +	scrub_rbio(container_of(work, struct btrfs_raid_bio, work));
>   }
>   
>   void raid56_parity_submit_scrub_rbio(struct btrfs_raid_bio *rbio)

Re: small raid56 cleanups v3

[Johannes Thumshirn]

On 11.01.23 07:24, Christoph Hellwig wrote:
> Hi all,
> 
> this series has a few trivial code cleanups for the raid56 code while
> I looked at it for another project.

Apart from a small nit that can be a follow up, the series looks good 
to me.

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

Re: small raid56 cleanups v3

[David Sterba]

On Wed, Jan 11, 2023 at 07:23:24AM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series has a few trivial code cleanups for the raid56 code while
> I looked at it for another project.
> 
> Changes since v2:
>  - lots of reshuffling based on feedback from qu
> 
> Changes since v1:
>  - cleanup more of the work handlers
>  - cleanup cleaning up unused bios
> 
> Diffstat:
>  raid56.c |  324 ++++++++++++++++++++++-----------------------------------------
>  1 file changed, 114 insertions(+), 210 deletions(-)

Added to misc-next, with some subject adjustments, thanks.