small raid56 cleanups

small raid56 cleanups

[Christoph Hellwig]

Hi all,

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

Diffstat:
 raid56.c |   91 ++++++++++++++++++++++++---------------------------------------
 1 file changed, 36 insertions(+), 55 deletions(-)

[PATCH 1/3] 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>
---
 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/3] 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>
---
 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/3] 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 5035e2b20a5e02..14d71076a222f9 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2275,7 +2275,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;
@@ -2287,7 +2287,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
@@ -2300,13 +2300,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;
 	}
 
 	/*
@@ -2339,7 +2339,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));
@@ -2356,32 +2356,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 1/3] btrfs: cleanup raid56_parity_write

[Qu Wenruo]

On 2022/12/12 15:06, Christoph Hellwig wrote:
> 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>

I originally thought the goto way would save some lines of code, but it 
turns out to be the opposite.

Thanks,
Qu

> ---
>   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,

Re: [PATCH 2/3] btrfs: cleanup rmw_rbio

[Qu Wenruo]

On 2022/12/12 15:06, Christoph Hellwig wrote:
> 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>

Thanks,
Qu
> ---
>   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

Re: [PATCH 3/3] btrfs: call rbio_orig_end_io from rmw_rbio

[Qu Wenruo]

On 2022/12/12 15:06, 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>

This changed the schema, as all work functions, rmw_rbio(), 
recover_rbio(), scrub_rbio() all follows the same return error, and let 
the caller to call rbio_orig_end_io().

I'm not against the change, but it's better to change all *_rbio() 
functions to follow the same behavior instead.

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 5035e2b20a5e02..14d71076a222f9 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2275,7 +2275,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;
> @@ -2287,7 +2287,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
> @@ -2300,13 +2300,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;
>   	}
>   
>   	/*
> @@ -2339,7 +2339,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));
> @@ -2356,32 +2356,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));
>   }
>   
>   /*

Re: [PATCH 3/3] btrfs: call rbio_orig_end_io from rmw_rbio

[Christoph Hellwig]

On Mon, Dec 12, 2022 at 03:39:57PM +0800, Qu Wenruo wrote:
> This changed the schema, as all work functions, rmw_rbio(), recover_rbio(), 
> scrub_rbio() all follows the same return error, and let the caller to call 
> rbio_orig_end_io().
>
> I'm not against the change, but it's better to change all *_rbio() 
> functions to follow the same behavior instead.

I hadn't looked at the other work items, but yes it seems like they
would benefit from a similar change.