lkml.org 
[lkml]   [2023]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH -next] radi10: fix leak of 'r10bio->remaining' for recovery
From
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

\
 
 \ /
  Last update: 2023-03-27 00:46    [W:1.365 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site