[PATCH -next] radi10: fix leak of 'r10bio->remaining' for recovery

[PATCH -next] radi10: fix leak of 'r10bio->remaining' for recovery

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

raid10_sync_request() will add 'r10bio->remaining' for both rdev and
replacement rdev. However, if the read io failed,
recovery_request_write() will return without issuring the write io, in
this case, end_sync_request() is only called once and 'remaining' is
leaked, which will cause io hang.

Fix the probleming by decreasing 'remaining' according to if 'bio' and
'repl_bio' is valid.

Fixes: 24afd80d99f8 ("md/raid10: handle recovery of replacement devices.")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid10.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index a8b5fecef136..f7002a1aa9cf 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2611,11 +2611,22 @@ static void recovery_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 {
 	struct r10conf *conf = mddev->private;
 	int d;
-	struct bio *wbio, *wbio2;
+	struct bio *wbio = r10_bio->devs[1].bio;
+	struct bio *wbio2 = r10_bio->devs[1].repl_bio;
+
+	/* Need to test wbio2->bi_end_io before we call
+	 * submit_bio_noacct as if the former is NULL,
+	 * the latter is free to free wbio2.
+	 */
+	if (wbio2 && !wbio2->bi_end_io)
+		wbio2 = NULL;
 
 	if (!test_bit(R10BIO_Uptodate, &r10_bio->state)) {
 		fix_recovery_read_error(r10_bio);
-		end_sync_request(r10_bio);
+		if (wbio->bi_end_io)
+			end_sync_request(r10_bio);
+		if (wbio2)
+			end_sync_request(r10_bio);
 		return;
 	}
 
@@ -2624,14 +2635,6 @@ static void recovery_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 	 * and submit the write request
 	 */
 	d = r10_bio->devs[1].devnum;
-	wbio = r10_bio->devs[1].bio;
-	wbio2 = r10_bio->devs[1].repl_bio;
-	/* Need to test wbio2->bi_end_io before we call
-	 * submit_bio_noacct as if the former is NULL,
-	 * the latter is free to free wbio2.
-	 */
-	if (wbio2 && !wbio2->bi_end_io)
-		wbio2 = NULL;
 	if (wbio->bi_end_io) {
 		atomic_inc(&conf->mirrors[d].rdev->nr_pending);
 		md_sync_acct(conf->mirrors[d].rdev->bdev, bio_sectors(wbio));
-- 
2.31.1

Re: [PATCH -next] radi10: fix leak of 'r10bio->remaining' for recovery

[Paul Menzel]

Dear Yu,


Thank you for your patch.

Am 07.03.23 um 03:27 schrieb Yu Kuai:
> From: Yu Kuai <yukuai3@huawei.com>

There is a small typo in the prefix of the commit message summary: raid10.

It also seems common to use md/raid10 as prefix.

> raid10_sync_request() will add 'r10bio->remaining' for both rdev and
> replacement rdev. However, if the read io failed,

fails (present tense for problem description/summary)

> recovery_request_write() will return without issuring the write io, in

1.  return*s*
2.  assuring?

> this case, end_sync_request() is only called once and 'remaining' is
> leaked, which will cause io hang.

leaked, causing an io hang.

> Fix the probleming by decreasing 'remaining' according to if 'bio' and

problem

> 'repl_bio' is valid.
> 
> Fixes: 24afd80d99f8 ("md/raid10: handle recovery of replacement devices.")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   drivers/md/raid10.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index a8b5fecef136..f7002a1aa9cf 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2611,11 +2611,22 @@ static void recovery_request_write(struct mddev *mddev, struct r10bio *r10_bio)
>   {
>   	struct r10conf *conf = mddev->private;
>   	int d;
> -	struct bio *wbio, *wbio2;
> +	struct bio *wbio = r10_bio->devs[1].bio;
> +	struct bio *wbio2 = r10_bio->devs[1].repl_bio;
> +
> +	/* Need to test wbio2->bi_end_io before we call
> +	 * submit_bio_noacct as if the former is NULL,
> +	 * the latter is free to free wbio2.
> +	 */
> +	if (wbio2 && !wbio2->bi_end_io)
> +		wbio2 = NULL;
>   
>   	if (!test_bit(R10BIO_Uptodate, &r10_bio->state)) {
>   		fix_recovery_read_error(r10_bio);
> -		end_sync_request(r10_bio);
> +		if (wbio->bi_end_io)
> +			end_sync_request(r10_bio);
> +		if (wbio2)
> +			end_sync_request(r10_bio);
>   		return;
>   	}
>   
> @@ -2624,14 +2635,6 @@ static void recovery_request_write(struct mddev *mddev, struct r10bio *r10_bio)
>   	 * and submit the write request
>   	 */
>   	d = r10_bio->devs[1].devnum;
> -	wbio = r10_bio->devs[1].bio;
> -	wbio2 = r10_bio->devs[1].repl_bio;
> -	/* Need to test wbio2->bi_end_io before we call
> -	 * submit_bio_noacct as if the former is NULL,
> -	 * the latter is free to free wbio2.
> -	 */
> -	if (wbio2 && !wbio2->bi_end_io)
> -		wbio2 = NULL;
>   	if (wbio->bi_end_io) {
>   		atomic_inc(&conf->mirrors[d].rdev->nr_pending);
>   		md_sync_acct(conf->mirrors[d].rdev->bdev, bio_sectors(wbio));


Kind regards,

Paul

Re: [PATCH -next] radi10: fix leak of 'r10bio->remaining' for recovery

[Yu Kuai]

Hi,

在 2023/03/07 17:53, Paul Menzel 写道:
> Dear Yu,
> 
> 
> Thank you for your patch.
> 
> Am 07.03.23 um 03:27 schrieb Yu Kuai:
>> From: Yu Kuai <yukuai3@huawei.com>
> 
> There is a small typo in the prefix of the commit message summary: raid10.
> 
> It also seems common to use md/raid10 as prefix.
> 
>> raid10_sync_request() will add 'r10bio->remaining' for both rdev and
>> replacement rdev. However, if the read io failed,
> 
> fails (present tense for problem description/summary)
> 
>> recovery_request_write() will return without issuring the write io, in
> 
> 1.  return*s*
> 2.  assuring?
> 
>> this case, end_sync_request() is only called once and 'remaining' is
>> leaked, which will cause io hang.
> 
> leaked, causing an io hang.
> 
>> Fix the probleming by decreasing 'remaining' according to if 'bio' and
> 
> problem
> 

Thanks for those advices, I'll send a new version after code changes is
reviewed.

Thanks,
Kuai

>> 'repl_bio' is valid.
>>
>> Fixes: 24afd80d99f8 ("md/raid10: handle recovery of replacement 
>> devices.")
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/raid10.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index a8b5fecef136..f7002a1aa9cf 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -2611,11 +2611,22 @@ static void recovery_request_write(struct 
>> mddev *mddev, struct r10bio *r10_bio)
>>   {
>>       struct r10conf *conf = mddev->private;
>>       int d;
>> -    struct bio *wbio, *wbio2;
>> +    struct bio *wbio = r10_bio->devs[1].bio;
>> +    struct bio *wbio2 = r10_bio->devs[1].repl_bio;
>> +
>> +    /* Need to test wbio2->bi_end_io before we call
>> +     * submit_bio_noacct as if the former is NULL,
>> +     * the latter is free to free wbio2.
>> +     */
>> +    if (wbio2 && !wbio2->bi_end_io)
>> +        wbio2 = NULL;
>>       if (!test_bit(R10BIO_Uptodate, &r10_bio->state)) {
>>           fix_recovery_read_error(r10_bio);
>> -        end_sync_request(r10_bio);
>> +        if (wbio->bi_end_io)
>> +            end_sync_request(r10_bio);
>> +        if (wbio2)
>> +            end_sync_request(r10_bio);
>>           return;
>>       }
>> @@ -2624,14 +2635,6 @@ static void recovery_request_write(struct mddev 
>> *mddev, struct r10bio *r10_bio)
>>        * and submit the write request
>>        */
>>       d = r10_bio->devs[1].devnum;
>> -    wbio = r10_bio->devs[1].bio;
>> -    wbio2 = r10_bio->devs[1].repl_bio;
>> -    /* Need to test wbio2->bi_end_io before we call
>> -     * submit_bio_noacct as if the former is NULL,
>> -     * the latter is free to free wbio2.
>> -     */
>> -    if (wbio2 && !wbio2->bi_end_io)
>> -        wbio2 = NULL;
>>       if (wbio->bi_end_io) {
>>           atomic_inc(&conf->mirrors[d].rdev->nr_pending);
>>           md_sync_acct(conf->mirrors[d].rdev->bdev, bio_sectors(wbio));
> 
> 
> Kind regards,
> 
> Paul
> 
> .
>