Re: [PATCH v4 6/9] md/raid1,raid10: Fix missing retries Failfast write bios on no-bbl rdevs

[Li Nan]

在 2025/9/19 0:23, Kenta Akagi 写道:
> On 2025/09/18 15:58, Li Nan wrote:
> > 在 2025/9/17 21:33, Kenta Akagi 写道:
> > > On 2025/09/17 19:06, Li Nan wrote:
> > > > 在 2025/9/15 11:42, Kenta Akagi 写道:
> > > > > In the current implementation, write failures are not retried on rdevs
> > > > > with badblocks disabled. This is because narrow_write_error, which issues
> > > > > retry bios, immediately returns when badblocks are disabled. As a result,
> > > > > a single write failure on such an rdev will immediately mark it as Faulty.
> > > >
> > > > IMO, there's no need to add extra logic for scenarios where badblocks
> > > > is not enabled. Do you have real-world scenarios where badblocks is
> > > > disabled?
> > >
> > > No, badblocks are enabled in my environment.
> > > I'm fine if it's not added, but I still think it's worth adding WARN_ON like:
> > >
> > > @@ -2553,13 +2554,17 @@ static bool narrow_write_error(struct r1bio *r1_bio, int i)
> > >     fail = true;
> > > + WARN_ON( (bio->bi_opf & MD_FAILFAST) && (rdev->badblocks.shift < 0) );
> > >     if (!narrow_write_error(r1_bio, m))
> > >
> > > What do you think?
> >
> > How about this?
> >
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -2522,10 +2522,11 @@ static bool narrow_write_error(struct r1bio *r1_bio, int i)
> >          bool ok = true;
> >
> >          if (rdev->badblocks.shift < 0)
> > -               return false;
> > +               block_sectors = bdev_logical_block_size(rdev->bdev) >> 9;
> > +       else
> > +               block_sectors = roundup(1 << rdev->badblocks.shift,
> > +                                       bdev_logical_block_size(rdev->bdev) >> 9);
> >
> > -       block_sectors = roundup(1 << rdev->badblocks.shift,
> > -                               bdev_logical_block_size(rdev->bdev) >> 9);
> >          sector = r1_bio->sector;
> >          sectors = ((sector + block_sectors)
> >                     & ~(sector_t)(block_sectors - 1))
> >
> > rdev_set_badblocks() checks shift, too. rdev is marked to Faulty if setting
> > badblocks fails.
>
> Sounds good. If badblocks are disabled, rdev_set_badblocks() returns false, so there
> should be no problem.
>
> Can this be included in the cleanup?
> https://lore.kernel.org/linux-raid/20250917093508.456790-3-linan666@xxxxxxxxxxxxxxx/T/#u
>
> or should I resend this patch as proposed?

Yes, please resend. I will rewrite my patch.

--
Thanks,
Nan