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

[Yu Kuai <yukuai1@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> > ---
> >   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
>
> .