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

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

Hi Paul,

On Mon, Apr 27, 2026 at 16:49 +0200, Paul Menzel wrote:
> Dear Abd-Alrhman,
>
>
> Thank you for your patch.
>

RAID 10 seems to have a similar issue, i will fix it too.

> Am 27.04.26 um 12:34 schrieb 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().
>> 
>> 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.
>
> Do you have a reproducer?
>
>> 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
>
> I suggest to always share the URL (lore.kernel.org), when referencing 
> another thread. If relevant, maybe even reference your message with a 
> Link: tag in the commit message.
>
>> ---
>>   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.
>
> s/but/But/
>
>>   	 */
>> -	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
>
> … we must read …
>
>> +	 * all the sectors or none.
>>   	 */
>> +	max_sectors = r1bio_existed;
>
> Excuse my ignorance, but I do not get why a bool is assigned to an int 
> representing the maximum sector value.
>
>>   	rdisk = read_balance(conf, r1_bio, &max_sectors);
>>   	if (rdisk < 0) {
>>   		/* couldn't find anywhere to read from */
>
>
> Kind regards,
>
> Paul

-- 
Best Regards,
Abd-Alrhman