Re: [PATCH] md/raid1: fix bio splitting in raid1 thread to avoid recursion and deadlock

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

Hi Kuai,

Thank you for the feedback.

On Tue, Apr 28, 2026 at 16:54 +0800, Yu Kuai wrote:
> Hi,
>
> 在 2026/4/27 18:34, Abd-Alrhman Masalkhi 写道:
>> Splitting a bio while executing in the raid1 thread can lead to
>> recursion, as task->bio_list is NULL in this context.
>>
>> In addition, resubmitting an md_cloned_bio after splitting may lead to
>> a deadlock if the array is suspended before the md driver calls
>> percpu_ref_tryget_live(&mddev->active_io) on it's path to
>> pers->make_request().
>
> I don't understand, I agree this is problematic in the suspend case, but
> what's wrong with task->bio_list being NULL? This can only cause the reverse
> order because the split bio will submit first. However this is not a big deal
> as this is the slow error patch.
>
> If suspend is the only problem here, the simple fix is to add checking
> in md_handle_request().
>

I meant that when current->bio_list is NULL, raid1_read_request()
can recurse into itself, as shown in the following trace-cmd output:

	  raid1_read_request() {                  <---
		bio_submit_split_bioset() {
		  bio_split() {..}
		  bio_chain();
		  submit_bio_noacct_nocheck() {
			__submit_bio() {
			  md_submit_bio() {
				md_handle_request() {
				  raid1_make_request() {
					raid1_read_request() {    <---
					  md_account_bio() { 

If this behavior is not an issue, I will follow your suggestion and
only add the check in md_handle_request().

>>
>> Avoid splitting the bio in this context and require that it is either
>> read in full or not at all.
>>
>> This prevents recursion and avoids potential deadlocks during array
>> suspension.
>>
>> Fixes: 689389a06ce7 ("md/raid1: simplify handle_read_error().")
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> I sent an email about this issue two days ago, but at the time I was not
>> sure whether it was a real problem or a misunderstanding on my part.
>>
>> After further analysis, it appears that this issue can occur.
>>
>> Apologies for the earlier confusion, and thank you for your time.
>>
>> Abd-Alrhman
>> ---
>>   drivers/md/raid1.c | 33 ++++++++++++++++++++++++---------
>>   1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index cc9914bd15c1..14f6d6625811 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -607,7 +607,7 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>>   
>>   		/* choose the first disk even if it has some bad blocks. */
>>   		read_len = raid1_check_read_range(rdev, this_sector, &len);
>> -		if (read_len > 0) {
>> +		if (read_len > 0 && (!*max_sectors || read_len == r1_bio->sectors)) {
>>   			update_read_sectors(conf, disk, this_sector, read_len);
>>   			*max_sectors = read_len;
>>   			return disk;
>> @@ -704,8 +704,13 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>>   	}
>>   
>>   	if (bb_disk != -1) {
>> -		*max_sectors = bb_read_len;
>> -		update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
>> +		if (!*max_sectors || bb_read_len == r1_bio->sectors) {
>> +			*max_sectors = bb_read_len;
>> +			update_read_sectors(conf, bb_disk, this_sector,
>> +					    bb_read_len);
>> +		} else {
>> +			bb_disk = -1;
>> +		}
>>   	}
>>   
>>   	return bb_disk;
>> @@ -852,8 +857,9 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>>    * disks and disks with bad blocks for now. Only pay attention to key disk
>>    * choice.
>>    *
>> - * 3) If we've made it this far, now look for disks with bad blocks and choose
>> - * the one with most number of sectors.
>> + * 3) If we've made it this far and *max_sectors is 0 (i.e., we are tolerant
>> + * of bad blocks), look for disks with bad blocks and choose the one with
>> + * the most sectors.
>>    *
>>    * 4) If we are all the way at the end, we have no choice but to use a disk even
>>    * if it is write mostly.
>> @@ -882,11 +888,13 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio,
>>   	/*
>>   	 * If we are here it means we didn't find a perfectly good disk so
>>   	 * now spend a bit more time trying to find one with the most good
>> -	 * sectors.
>> +	 * sectors. but only if we are tolerant of bad blocks.
>>   	 */
>> -	disk = choose_bb_rdev(conf, r1_bio, max_sectors);
>> -	if (disk >= 0)
>> -		return disk;
>> +	if (!*max_sectors) {
>> +		disk = choose_bb_rdev(conf, r1_bio, max_sectors);
>> +		if (disk >= 0)
>> +			return disk;
>> +	}
>>   
>>   	return choose_slow_rdev(conf, r1_bio, max_sectors);
>>   }
>> @@ -1346,7 +1354,14 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>>   	/*
>>   	 * make_request() can abort the operation when read-ahead is being
>>   	 * used and no empty request is available.
>> +	 *
>> +	 * If we allow splitting the bio while executing in the raid1 thread,
>> +	 * we may end up recursing (current->bio_list is NULL), and we might
>> +	 * also deadlock if we try to suspend the array, since we are
>> +	 * resubmitting an md_cloned_bio. Therefore, we must be read either
>> +	 * all the sectors or none.
>>   	 */
>> +	max_sectors = r1bio_existed;
>>   	rdisk = read_balance(conf, r1_bio, &max_sectors);
>>   	if (rdisk < 0) {
>>   		/* couldn't find anywhere to read from */
>
> -- 
> Thansk,
> Kuai

-- 
Best Regards,
Abd-Alrhman