small raid56 cleanups v2

small raid56 cleanups v2

[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 v1:
 - cleanup more of the work handlers
 - cleanup cleaning up unused bios

Diffstat:
 raid56.c |  167 +++++++++++++++++++++------------------------------------------
 1 file changed, 58 insertions(+), 109 deletions(-)

[PATCH 1/8] 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 2d90a6b5eb00e3..5603ba1af55584 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 2/8] 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 5603ba1af55584..5035e2b20a5e02 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 3/8] btrfs: cleanup rmw_read_wait_recover

[Christoph Hellwig]

rmw_assemble_read_bios already cleans up the bio_list on failure, so the
loop to do so in rmw_read_wait_recover will never do anything and can be
removed.  Also initialize the bio_list at initialization time, and
directly return the value from recover_sectors instead of assigning it to
ret first.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 5035e2b20a5e02..e0966126ab27a4 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2189,12 +2189,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;
-	struct bio *bio;
+	struct bio_list bio_list = BIO_EMPTY_LIST;
 	int ret;
 
-	bio_list_init(&bio_list);
-
 	/*
 	 * Fill the data csums we need for data verification.  We need to fill
 	 * the csum_bitmap/csum_buf first, as our endio function will try to
@@ -2204,7 +2201,7 @@ static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
 
 	ret = rmw_assemble_read_bios(rbio, &bio_list);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	submit_read_bios(rbio, &bio_list);
 	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
@@ -2213,13 +2210,7 @@ static int rmw_read_wait_recover(struct btrfs_raid_bio *rbio)
 	 * 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:
-	while ((bio = bio_list_pop(&bio_list)))
-		bio_put(bio);
-
-	return ret;
+	return recover_sectors(rbio);
 }
 
 static void raid_wait_write_end_io(struct bio *bio)
-- 
2.35.1

[PATCH 4/8] btrfs: cleanup recover_rbio

[Christoph Hellwig]

The bio_list is only filled by recover_assemble_read_bios when
successful, so don't try to walk it and put the bios on any
failure before the successful call to recover_assemble_read_bios.
Also initialize bio_list at initialization time.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index e0966126ab27a4..5dab0685e17dd5 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1987,7 +1987,7 @@ static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
 
 static int recover_rbio(struct btrfs_raid_bio *rbio)
 {
-	struct bio_list bio_list;
+	struct bio_list bio_list = BIO_EMPTY_LIST;
 	struct bio *bio;
 	int ret;
 
@@ -1996,28 +1996,24 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
 	 * 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;
+		return ret;
 
 	index_rbio_pages(rbio);
 
 	ret = recover_assemble_read_bios(rbio, &bio_list);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	submit_read_bios(rbio, &bio_list);
 	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
 
 	ret = recover_sectors(rbio);
-
-out:
 	while ((bio = bio_list_pop(&bio_list)))
 		bio_put(bio);
-
 	return ret;
 }
 
-- 
2.35.1

Re: [PATCH 4/8] btrfs: cleanup recover_rbio

[Qu Wenruo]

On 2022/12/13 16:41, Christoph Hellwig wrote:
> The bio_list is only filled by recover_assemble_read_bios when
> successful, so don't try to walk it and put the bios on any
> failure before the successful call to recover_assemble_read_bios.
> Also initialize bio_list at initialization time.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/raid56.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index e0966126ab27a4..5dab0685e17dd5 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1987,7 +1987,7 @@ static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   
>   static int recover_rbio(struct btrfs_raid_bio *rbio)
>   {
> -	struct bio_list bio_list;
> +	struct bio_list bio_list = BIO_EMPTY_LIST;
>   	struct bio *bio;
>   	int ret;
>   
> @@ -1996,28 +1996,24 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
>   	 * 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;
> +		return ret;
>   
>   	index_rbio_pages(rbio);
>   
>   	ret = recover_assemble_read_bios(rbio, &bio_list);
>   	if (ret < 0)
> -		goto out;
> +		return ret;
>   
>   	submit_read_bios(rbio, &bio_list);
>   	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
>   
>   	ret = recover_sectors(rbio);
> -
> -out:
>   	while ((bio = bio_list_pop(&bio_list)))
>   		bio_put(bio);
> -
>   	return ret;
>   }
>   

[PATCH 5/8] btrfs: cleanup scrub_rbio

[Christoph Hellwig]

The bio_list is only filled by scrub_assemble_read_bios when
successful, so don't try to walk it and put the bios on any
failure before the successful call to recover_assemble_read_bios.
Also initialize bio_list at initialization time.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 5dab0685e17dd5..2a5857d42fff20 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2754,31 +2754,32 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
 
 static int scrub_rbio(struct btrfs_raid_bio *rbio)
 {
+	struct bio_list bio_list = BIO_EMPTY_LIST;
 	bool need_check = false;
-	struct bio_list bio_list;
 	int sector_nr;
 	int ret;
 	struct bio *bio;
 
-	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);
 	if (ret < 0)
-		goto cleanup;
+		return ret;
 
 	submit_read_bios(rbio, &bio_list);
 	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
 
 	/* We may have some failures, recover the failed sectors first. */
 	ret = recover_scrub_rbio(rbio);
-	if (ret < 0)
-		goto cleanup;
+	if (ret < 0) {
+		while ((bio = bio_list_pop(&bio_list)))
+			bio_put(bio);
+		return ret;
+	}
 
 	/*
 	 * We have every sector properly prepared. Can finish the scrub
@@ -2796,12 +2797,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
 		}
 	}
 	return ret;
-
-cleanup:
-	while ((bio = bio_list_pop(&bio_list)))
-		bio_put(bio);
-
-	return ret;
 }
 
 static void scrub_rbio_work_locked(struct work_struct *work)
-- 
2.35.1

Re: [PATCH 5/8] btrfs: cleanup scrub_rbio

[Qu Wenruo]

On 2022/12/13 16:41, Christoph Hellwig wrote:
> The bio_list is only filled by scrub_assemble_read_bios when
> successful, so don't try to walk it and put the bios on any
> failure before the successful call to recover_assemble_read_bios.
> Also initialize bio_list at initialization time.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/raid56.c | 21 ++++++++-------------
>   1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 5dab0685e17dd5..2a5857d42fff20 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2754,31 +2754,32 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   
>   static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   {
> +	struct bio_list bio_list = BIO_EMPTY_LIST;
>   	bool need_check = false;
> -	struct bio_list bio_list;
>   	int sector_nr;
>   	int ret;
>   	struct bio *bio;
>   
> -	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);
>   	if (ret < 0)
> -		goto cleanup;
> +		return ret;
>   
>   	submit_read_bios(rbio, &bio_list);
>   	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
>   
>   	/* We may have some failures, recover the failed sectors first. */
>   	ret = recover_scrub_rbio(rbio);
> -	if (ret < 0)
> -		goto cleanup;
> +	if (ret < 0) {
> +		while ((bio = bio_list_pop(&bio_list)))
> +			bio_put(bio);
> +		return ret;
> +	}

Do we still need the cleanup? IIRC after submit_read_bios() (or be more 
safe, after wait_event()), we should no longer touch @bio_list anymore.

Thus the cleanup itself is no longer needed AFAIK.

Thanks,
Qu

>   
>   	/*
>   	 * We have every sector properly prepared. Can finish the scrub
> @@ -2796,12 +2797,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   		}
>   	}
>   	return ret;
> -
> -cleanup:
> -	while ((bio = bio_list_pop(&bio_list)))
> -		bio_put(bio);
> -
> -	return ret;
>   }
>   
>   static void scrub_rbio_work_locked(struct work_struct *work)

Re: [PATCH 5/8] btrfs: cleanup scrub_rbio

[Christoph Hellwig]

On Tue, Dec 13, 2022 at 04:53:33PM +0800, Qu Wenruo wrote:
>>     	ret = scrub_assemble_read_bios(rbio, &bio_list);
>>   	if (ret < 0)
>> -		goto cleanup;
>> +		return ret;
>>     	submit_read_bios(rbio, &bio_list);
>>   	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
>>     	/* We may have some failures, recover the failed sectors first. */
>>   	ret = recover_scrub_rbio(rbio);
>> -	if (ret < 0)
>> -		goto cleanup;
>> +	if (ret < 0) {
>> +		while ((bio = bio_list_pop(&bio_list)))
>> +			bio_put(bio);
>> +		return ret;
>> +	}
>
> Do we still need the cleanup? IIRC after submit_read_bios() (or be more 
> safe, after wait_event()), we should no longer touch @bio_list anymore.

Oh, true.  submit_read_bios does the list_pop, so we don't need
this here, and in recover_rbio either.  And looking at it a little more
I think the are could use even more cleanup by:

 - moving the wait_event for stripes_pending into submit_read_bios.
 - moving the submit_read_bios *_assemble_read_bios and stop passing
   the bio_list entirely, which removes all the confusion about
   who cleans it up.

What do you think of this series (still needs testing before I can
post it):

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-raid56-cleanups

Re: [PATCH 5/8] btrfs: cleanup scrub_rbio

[Qu Wenruo]

On 2022/12/13 21:24, Christoph Hellwig wrote:
> On Tue, Dec 13, 2022 at 04:53:33PM +0800, Qu Wenruo wrote:
>>>      	ret = scrub_assemble_read_bios(rbio, &bio_list);
>>>    	if (ret < 0)
>>> -		goto cleanup;
>>> +		return ret;
>>>      	submit_read_bios(rbio, &bio_list);
>>>    	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
>>>      	/* We may have some failures, recover the failed sectors first. */
>>>    	ret = recover_scrub_rbio(rbio);
>>> -	if (ret < 0)
>>> -		goto cleanup;
>>> +	if (ret < 0) {
>>> +		while ((bio = bio_list_pop(&bio_list)))
>>> +			bio_put(bio);
>>> +		return ret;
>>> +	}
>>
>> Do we still need the cleanup? IIRC after submit_read_bios() (or be more
>> safe, after wait_event()), we should no longer touch @bio_list anymore.
> 
> Oh, true.  submit_read_bios does the list_pop, so we don't need
> this here, and in recover_rbio either.  And looking at it a little more
> I think the are could use even more cleanup by:
> 
>   - moving the wait_event for stripes_pending into submit_read_bios.
>   - moving the submit_read_bios *_assemble_read_bios and stop passing
>     the bio_list entirely, which removes all the confusion about
>     who cleans it up.
> 
> What do you think of this series (still needs testing before I can
> post it):
> 
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-raid56-cleanups

The code change all looks good, I'd just prefer some naming changes.

One personal thing is, I'd prefer "_wait_" for functions that may wait 
for IOs. Thus submit_read_bios() may be better renamed to 
submit_read_wait_bios().
(bios means it still directly handle a bio list, or just 
submit_read_wait_bio_list()?)

Another changes is, since there is no bio_list out of the 
<work>_read_bios(), the "bios" naming can be removed.
Something like <work>_read_wait() may be a little better.

Thanks,
Qu

Re: [PATCH 5/8] btrfs: cleanup scrub_rbio

[Christoph Hellwig]

On Wed, Dec 14, 2022 at 07:32:00AM +0800, Qu Wenruo wrote:
>
> One personal thing is, I'd prefer "_wait_" for functions that may wait for 
> IOs. Thus submit_read_bios() may be better renamed to 
> submit_read_wait_bios().
> (bios means it still directly handle a bio list, or just 
> submit_read_wait_bio_list()?)

I can do either one.

>
> Another changes is, since there is no bio_list out of the 
> <work>_read_bios(), the "bios" naming can be removed.
> Something like <work>_read_wait() may be a little better.

Sure.

[PATCH 6/8] 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 2a5857d42fff20..2432c2d7fcbed0 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2262,7 +2262,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;
@@ -2274,7 +2274,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
@@ -2287,13 +2287,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;
 	}
 
 	/*
@@ -2326,7 +2326,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));
@@ -2343,32 +2343,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 6/8] btrfs: call rbio_orig_end_io from rmw_rbio

[Qu Wenruo]

On 2022/12/13 16:41, Christoph Hellwig wrote:
> Both callers of rmv_rbio call rbio_orig_end_io right after it, so
> move the call into the shared function.

s/rmv_rbio/rmw_rbio/.

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

Thanks,
Qu
> 
> 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 2a5857d42fff20..2432c2d7fcbed0 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2262,7 +2262,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;
> @@ -2274,7 +2274,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
> @@ -2287,13 +2287,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;
>   	}
>   
>   	/*
> @@ -2326,7 +2326,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));
> @@ -2343,32 +2343,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 7/8] 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 | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 2432c2d7fcbed0..b2e02f02294163 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1985,7 +1985,7 @@ static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
 	return -EIO;
 }
 
-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;
 	struct bio *bio;
@@ -2000,13 +2000,13 @@ 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);
 
 	ret = recover_assemble_read_bios(rbio, &bio_list);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	submit_read_bios(rbio, &bio_list);
 	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
@@ -2014,32 +2014,22 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
 	ret = recover_sectors(rbio);
 	while ((bio = bio_list_pop(&bio_list)))
 		bio_put(bio);
-	return ret;
+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 7/8] btrfs: call rbio_orig_end_io from recover_rbio

[Qu Wenruo]

On 2022/12/13 16:41, 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 | 26 ++++++++------------------
>   1 file changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 2432c2d7fcbed0..b2e02f02294163 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1985,7 +1985,7 @@ static int recover_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   	return -EIO;
>   }
>   
> -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;
>   	struct bio *bio;
> @@ -2000,13 +2000,13 @@ 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);
>   
>   	ret = recover_assemble_read_bios(rbio, &bio_list);
>   	if (ret < 0)
> -		return ret;
> +		goto out;
>   
>   	submit_read_bios(rbio, &bio_list);
>   	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
> @@ -2014,32 +2014,22 @@ static int recover_rbio(struct btrfs_raid_bio *rbio)
>   	ret = recover_sectors(rbio);
>   	while ((bio = bio_list_pop(&bio_list)))
>   		bio_put(bio);
> -	return ret;
> +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 8/8] 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 b2e02f02294163..4f2d63bfddae3c 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2732,7 +2732,7 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
 	return ret;
 }
 
-static int scrub_rbio(struct btrfs_raid_bio *rbio)
+static void scrub_rbio(struct btrfs_raid_bio *rbio)
 {
 	struct bio_list bio_list = BIO_EMPTY_LIST;
 	bool need_check = false;
@@ -2742,13 +2742,13 @@ 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, &bio_list);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	submit_read_bios(rbio, &bio_list);
 	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
@@ -2758,7 +2758,7 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
 	if (ret < 0) {
 		while ((bio = bio_list_pop(&bio_list)))
 			bio_put(bio);
-		return ret;
+		goto out;
 	}
 
 	/*
@@ -2776,17 +2776,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 8/8] btrfs: call rbio_orig_end_io from scrub_rbio

[Qu Wenruo]

On 2022/12/13 16:41, 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 b2e02f02294163..4f2d63bfddae3c 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2732,7 +2732,7 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   	return ret;
>   }
>   
> -static int scrub_rbio(struct btrfs_raid_bio *rbio)
> +static void scrub_rbio(struct btrfs_raid_bio *rbio)
>   {
>   	struct bio_list bio_list = BIO_EMPTY_LIST;
>   	bool need_check = false;
> @@ -2742,13 +2742,13 @@ 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, &bio_list);
>   	if (ret < 0)
> -		return ret;
> +		goto out;
>   
>   	submit_read_bios(rbio, &bio_list);
>   	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
> @@ -2758,7 +2758,7 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   	if (ret < 0) {
>   		while ((bio = bio_list_pop(&bio_list)))
>   			bio_put(bio);
> -		return ret;
> +		goto out;
>   	}
>   
>   	/*
> @@ -2776,17 +2776,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 v2

[Qu Wenruo]

On 2022/12/13 16:41, 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.

The series looks good overall, but I'm more interested in how btrfs 
RAID56 can help your other projects.

IIRC btrfs RAID56 should be less feature-full compared to things like 
dm-raid56, no write-intent/journal etc. (but I'm not a fan to read 
dm-raid56 code either)

And if you have other feedbacks or uncertainty, I'm pretty happy to 
improve the raid56 code.

Thanks,
Qu

> 
> Changes since v1:
>   - cleanup more of the work handlers
>   - cleanup cleaning up unused bios
> 
> Diffstat:
>   raid56.c |  167 +++++++++++++++++++++------------------------------------------
>   1 file changed, 58 insertions(+), 109 deletions(-)

Re: small raid56 cleanups v2

[Christoph Hellwig]

On Tue, Dec 13, 2022 at 04:59:35PM +0800, Qu Wenruo wrote:
> The series looks good overall, but I'm more interested in how btrfs RAID56 
> can help your other projects.

The project here is mostly to hand off to workqueue earlier in the btrfs
I/O completion path, and thus mostly avoiding irqsave spinlocking,
similar to what XFS has been doing for quite a while.