Messages in this thread | | | Date | Tue, 7 Mar 2023 10:53:27 +0100 | Subject | Re: [PATCH -next] radi10: fix leak of 'r10bio->remaining' for recovery | From | 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
| |