From: Yu Kuai <[email protected]>
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 <[email protected]>
---
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
Dear Yu,
Thank you for your patch.
Am 07.03.23 um 03:27 schrieb Yu Kuai:
> From: Yu Kuai <[email protected]>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>
> 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 <[email protected]>
>> ---
>> 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
>
> .
>