From: Yu Kuai <yukuai3@huawei.com>

Unify bio split code, and prepare to fix disordered split IO.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid1.c | 38 +++++++++++---------------------------
 drivers/md/raid1.h |  4 +++-
 2 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 29edb7b548f3..f8434049f9b1 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1317,7 +1317,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 	struct raid1_info *mirror;
 	struct bio *read_bio;
 	int max_sectors;
-	int rdisk, error;
+	int rdisk;
 	bool r1bio_existed = !!r1_bio;
 
 	/*
@@ -1376,18 +1376,13 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 	}
 
 	if (max_sectors < bio_sectors(bio)) {
-		struct bio *split = bio_split(bio, max_sectors,
-					      gfp, &conf->bio_split);
-
-		if (IS_ERR(split)) {
-			error = PTR_ERR(split);
+		bio = bio_submit_split_bioset(bio, max_sectors,
+					      &conf->bio_split);
+		if (!bio) {
+			set_bit(R1BIO_Returned, &r1_bio->state);
 			goto err_handle;
 		}
 
-		bio_chain(split, bio);
-		trace_block_split(split, bio->bi_iter.bi_sector);
-		submit_bio_noacct(bio);
-		bio = split;
 		r1_bio->master_bio = bio;
 		r1_bio->sectors = max_sectors;
 	}
@@ -1415,8 +1410,6 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 
 err_handle:
 	atomic_dec(&mirror->rdev->nr_pending);
-	bio->bi_status = errno_to_blk_status(error);
-	set_bit(R1BIO_Uptodate, &r1_bio->state);
 	raid_end_bio_io(r1_bio);
 }
 
@@ -1459,7 +1452,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 {
 	struct r1conf *conf = mddev->private;
 	struct r1bio *r1_bio;
-	int i, disks, k, error;
+	int i, disks, k;
 	unsigned long flags;
 	int first_clone;
 	int max_sectors;
@@ -1563,10 +1556,8 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 				 * complexity of supporting that is not worth
 				 * the benefit.
 				 */
-				if (bio->bi_opf & REQ_ATOMIC) {
-					error = -EIO;
+				if (bio->bi_opf & REQ_ATOMIC)
 					goto err_handle;
-				}
 
 				good_sectors = first_bad - r1_bio->sector;
 				if (good_sectors < max_sectors)
@@ -1586,18 +1577,13 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 		max_sectors = min_t(int, max_sectors,
 				    BIO_MAX_VECS * (PAGE_SIZE >> 9));
 	if (max_sectors < bio_sectors(bio)) {
-		struct bio *split = bio_split(bio, max_sectors,
-					      GFP_NOIO, &conf->bio_split);
-
-		if (IS_ERR(split)) {
-			error = PTR_ERR(split);
+		bio = bio_submit_split_bioset(bio, max_sectors,
+					      &conf->bio_split);
+		if (!bio) {
+			set_bit(R1BIO_Returned, &r1_bio->state);
 			goto err_handle;
 		}
 
-		bio_chain(split, bio);
-		trace_block_split(split, bio->bi_iter.bi_sector);
-		submit_bio_noacct(bio);
-		bio = split;
 		r1_bio->master_bio = bio;
 		r1_bio->sectors = max_sectors;
 	}
@@ -1687,8 +1673,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 		}
 	}
 
-	bio->bi_status = errno_to_blk_status(error);
-	set_bit(R1BIO_Uptodate, &r1_bio->state);
 	raid_end_bio_io(r1_bio);
 }
 
diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
index d236ef179cfb..2ebe35aaa534 100644
--- a/drivers/md/raid1.h
+++ b/drivers/md/raid1.h
@@ -178,7 +178,9 @@ enum r1bio_state {
  * any write was successful.  Otherwise we call when
  * any write-behind write succeeds, otherwise we call
  * with failure when last write completes (and all failed).
- * Record that bi_end_io was called with this flag...
+ *
+ * And for bio_split errors, record that bi_end_io was called
+ * with this flag...
  */
 	R1BIO_Returned,
 /* If a write for this request means we can clear some
-- 
2.39.2

On 9/1/25 12:32 PM, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Unify bio split code, and prepare to fix disordered split IO.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

[...]

> @@ -1586,18 +1577,13 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>  		max_sectors = min_t(int, max_sectors,
>  				    BIO_MAX_VECS * (PAGE_SIZE >> 9));
>  	if (max_sectors < bio_sectors(bio)) {
> -		struct bio *split = bio_split(bio, max_sectors,
> -					      GFP_NOIO, &conf->bio_split);
> -
> -		if (IS_ERR(split)) {
> -			error = PTR_ERR(split);
> +		bio = bio_submit_split_bioset(bio, max_sectors,
> +					      &conf->bio_split);
> +		if (!bio) {
> +			set_bit(R1BIO_Returned, &r1_bio->state);

Before it was "set_bit(R1BIO_Uptodate, &r1_bio->state);" that was done. Now it
is R1BIO_Returned that is set. The commit message does not mention this change
at all. Is this a bug fix ? If yes, that should be in a pre patch before the
conversion to using bio_submit_split_bioset().

>  			goto err_handle;
>  		}
>  
> -		bio_chain(split, bio);
> -		trace_block_split(split, bio->bi_iter.bi_sector);
> -		submit_bio_noacct(bio);
> -		bio = split;
>  		r1_bio->master_bio = bio;
>  		r1_bio->sectors = max_sectors;
>  	}
> @@ -1687,8 +1673,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>  		}
>  	}
>  
> -	bio->bi_status = errno_to_blk_status(error);
> -	set_bit(R1BIO_Uptodate, &r1_bio->state);
>  	raid_end_bio_io(r1_bio);
>  }


-- 
Damien Le Moal
Western Digital Research

Hi,

在 2025/09/01 14:43, Damien Le Moal 写道:
> On 9/1/25 12:32 PM, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Unify bio split code, and prepare to fix disordered split IO.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> 
> [...]
> 
>> @@ -1586,18 +1577,13 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>>   		max_sectors = min_t(int, max_sectors,
>>   				    BIO_MAX_VECS * (PAGE_SIZE >> 9));
>>   	if (max_sectors < bio_sectors(bio)) {
>> -		struct bio *split = bio_split(bio, max_sectors,
>> -					      GFP_NOIO, &conf->bio_split);
>> -
>> -		if (IS_ERR(split)) {
>> -			error = PTR_ERR(split);
>> +		bio = bio_submit_split_bioset(bio, max_sectors,
>> +					      &conf->bio_split);
>> +		if (!bio) {
>> +			set_bit(R1BIO_Returned, &r1_bio->state);
> 
> Before it was "set_bit(R1BIO_Uptodate, &r1_bio->state);" that was done. Now it
> is R1BIO_Returned that is set. The commit message does not mention this change
> at all. Is this a bug fix ? If yes, that should be in a pre patch before the
> conversion to using bio_submit_split_bioset().

There should be no functional changes, before the change we:

1) set bio->bi_status to split error value;
2) set R1BIO_Uptodate;
3) raid_end_bio_io() check R1BIO_Returned is not set, and call
call_bio_endio();
4) call_bio_endio() check R1BIO_Uptodate is already set, keep the
bio->bi_status that is by split error;

With this change:
1) bio_submit_split_bioset() already fail the bio will split error;
2) set R1BIO_Returned;
3) raid_end_bio_io() check R1BIO_Returned is set and do nothing with the
bio;

And the same with raid10 in patch 8,9;

Perhaps I'll emphasize there is no function changes and explain a bit.

Thanks,
Kuai
> 
>>   			goto err_handle;
>>   		}
>>   
>> -		bio_chain(split, bio);
>> -		trace_block_split(split, bio->bi_iter.bi_sector);
>> -		submit_bio_noacct(bio);
>> -		bio = split;
>>   		r1_bio->master_bio = bio;
>>   		r1_bio->sectors = max_sectors;
>>   	}
>> @@ -1687,8 +1673,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>>   		}
>>   	}
>>   
>> -	bio->bi_status = errno_to_blk_status(error);
>> -	set_bit(R1BIO_Uptodate, &r1_bio->state);
>>   	raid_end_bio_io(r1_bio);
>>   }
> 
> 

On Mon, Sep 01, 2025 at 04:03:22PM +0800, Yu Kuai wrote:
> With this change:
> 1) bio_submit_split_bioset() already fail the bio will split error;
> 2) set R1BIO_Returned;
> 3) raid_end_bio_io() check R1BIO_Returned is set and do nothing with the
> bio;
> 
> And the same with raid10 in patch 8,9;
> 
> Perhaps I'll emphasize there is no function changes and explain a bit.

Please spell the above in the commit log.  Or do the flag changes
separately so that it is even more clear.