Re: [PATCH v4 5/9] md/raid1,raid10: Set R{1,10}BIO_Uptodate when successful retry of a failed bio

[Li Nan]

在 2025/9/18 23:36, Kenta Akagi 写道:
> On 2025/09/18 15:39, Li Nan wrote:
> > 在 2025/9/17 21:20, Kenta Akagi 写道:
> >
> > > > >                 if (!narrow_write_error(r1_bio, m))
> > > > > -                md_error(conf->mddev,
> > > > > -                     conf->mirrors[m].rdev);
> > > > > +                md_error(conf->mddev, rdev);
> > > > >                     /* an I/O failed, we can't clear the bitmap */
> > > > > -            rdev_dec_pending(conf->mirrors[m].rdev,
> > > > > -                     conf->mddev);
> > > > > +            else if (test_bit(In_sync, &rdev->flags) &&
> > > > > +                 !test_bit(Faulty, &rdev->flags) &&
> > > > > +                 rdev_has_badblock(rdev,
> > > > > +                           r1_bio->sector,
> > > > > +                           r1_bio->sectors) == 0)
> > > >
> > > > Clear badblock and set R10BIO_Uptodate if rdev has badblock.
> > >
> > > narrow_write_error returns true when the write succeeds, or when the write
> > > fails but rdev_set_badblocks succeeds. Here, it determines that the re-write
> > > succeeded if there is no badblock in the sector to be written by r1_bio.
> > > So we should not call rdev_clear_badblocks here.
> >
> > I am trying to cleanup narrow_write_error():
> >
> > https://lore.kernel.org/linux-raid/20250917093508.456790-3-linan666@xxxxxxxxxxxxxxx/T/#u
> >
> > It may be clearer if narrow_write_error() returns true when all fix IO
> > succeeds.
> >
> > ```
> > @@ -2553,11 +2551,17 @@ static bool narrow_write_error(struct r1bio *r1_bio, int i)
> >                  bio_trim(wbio, sector - r1_bio->sector, sectors);
> >                  wbio->bi_iter.bi_sector += rdev->data_offset;
> >
> > -               if (submit_bio_wait(wbio) < 0)
> > -                       /* failure! */
> > -                       ok = rdev_set_badblocks(rdev, sector,
> > -                                               sectors, 0)
> > -                               && ok;
> > +               if (submit_bio_wait(wbio) < 0) {
> > +                       ok = false;
> > +                       if (rdev_set_badblocks(rdev, sector, sectors, 0)) {
> > +                               /*
> > +                                * Badblocks set failed, disk marked Faulty.
> > +                                * No further operations needed.
> > +                                */
> > +                               bio_put(wbio);
> > +                               break;
> > +                       }
> > +               }
> >
> >                  bio_put(wbio);
> >                  sect_to_write -= sectors;
> > ```
> >
> > We can clear badblocks and set R10BIO_Uptodate after it. What do you think?
>
> Thanks for the detailed explanation.
> narrow_write_error now returns whether the retry write succeeded.
> If rdev_set_badblock fails, narrow_write_error calls md_error. This looks good to me.
> And after narrow_write_error cleanup, rdev_clear_badblock should be called.
>
> BTW, I'm not familiar with the patch workflow. Should I submit my
> handle_write_finished patch separately after your cleanup patch merged?

Sure, please send it separately after my cleanup patch. Thanks.

> Thanks,
> Akagi
>
> > --
> > Thanks,
> > Nan
>
>
>
> .

--
Thanks,
Nan