Re: A sector-of-mismatch warning patch (was Re: Fault tolerance with badblocks)

[Shaohua Li <shli@kernel.org>]

On Fri, May 19, 2017 at 11:32:43AM +0100, Nix wrote:
> On 19 May 2017, NeilBrown said:
> 
> > On Tue, May 16 2017, Nix wrote:
> >
> >> On 16 May 2017, NeilBrown spake thusly:
> >>
> >>> Actually, I have another caveat.  I don't think we want these messages
> >>> during initial resync, or any resync.  Only during a 'check' or
> >>> 'repair'.
> >>> So add a check for MD_RECOVERY_REQUESTED or maybe for
> >>>   sh->sectors >= conf->mddev->recovery_cp
> >>
> >> I completely agree, but it's already inside MD_RECOVERY_CHECK:
> >>
> >> if (test_bit(MD_RECOVERY_CHECK, &conf->mddev->recovery)) {
> >>         /* don't try to repair!! */
> >>         set_bit(STRIPE_INSYNC, &sh->state);
> >>         pr_warn_ratelimited("%s: mismatch sector in range "
> >>                             "%llu-%llu\n", mdname(conf->mddev),
> >>                             (unsigned long long) sh->sector,
> >>                             (unsigned long long) sh->sector +
> >>                             STRIPE_SECTORS);
> >> } else {
> >>
> >> Doesn't that already mean that someone has explicitly triggered a check
> >> action?
> >
> > Uhmm... yeah.  I lose track of which flags me what exactly.
> > You log messages aren't generated when 'repair' is used, only when
> > 'check' is.
> > I can see why you might have chosen that, but I wonder if it is best.
> 
> I'm not sure what the point is of being told when repair is used: hey,
> there was an inconsistency here but there isn't any more! I suppose you
> could still use it to see if the repair did the right thing. My problem
> on that front was that I'm not sure what flag should be used to catch
> repair but not resync etc: everywhere else in the code, repair is in an
> unadorned else branch... is it the *lack* of MD_RECOVERY_CHECK and the
> presence of, uh, something else?
MD_RECOVERY_SYNC && MD_RECOVERY_REQUESTED && MD_RECOVERY_CHECK == check
MD_RECOVERY_SYNC && MD_RECOVERY_REQUESTED == repair
MD_RECOVERY_SYNC && !MD_RECOVERY_REQUESTED == resync

Don't see the poin to print the info for 'repair'. 'repair' already changes the
data, how could we use the info?

Thanks,
Shaohua