Hi Mariusz,
On Tue, Feb 15, 2022 at 6:06 AM Mariusz Tkaczyk
<mariusz.tkaczyk@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, 15 Feb 2022 11:43:34 +0800
> Guoqing Jiang <guoqing.jiang@xxxxxxxxx> wrote:
>
> > >> It also adopts md_error(), we only want to call .error_handler for
> > >> those levels. mddev->pers->sync_request is additionally checked,
> > >> its existence implies a level with redundancy.
> > >>
> > >> Usage of error_handler causes that disk failure can be requested
> > >> from userspace. User can fail the array via #mdadm --set-faulty
> > >> command. This is not safe and will be fixed in mdadm.
> > >> What is the safe issue here? It would betterr to post mdadm fix
> > >> together.
> > > We can and should block user from damaging raid even if it is
> > > recoverable. It is a regression.
> >
> > I don't follow, did you mean --set-fault from mdadm could "damaging
> > raid"?
>
> Yes, now it will be able to impose failed state. This is a regression I
> caused and I'm aware of that.
> >
> > > I will fix mdadm. I don't consider it as a big risk (because it is
> > > recoverable) so I focused on kernel part first.
> > >
> > >>> It is correctable because failed
> > >>> state is not recorded in the metadata. After next assembly array
> > >>> will be read-write again.
> > >> I don't think it is a problem, care to explain why it can't be RW
> > >> again?
> > > failed state is not recoverable in runtime, so you need to recreate
> > > array.
> >
> > IIUC, the failfast flag is supposed to be set during transient error
> > not permanent failure, the rdev (marked as failfast) need to be
> > revalidated and readded to array.
> >
>
> The device is never marked as failed. I checked write paths (because I
> introduced more aggressive policy for writes) and if we are in a
> critical array, failfast flag is not added to bio for both raid1 and
> radi10, see raid1_write_request().
> >
> > [ ... ]
> >
> > >>> + char *md_name = mdname(mddev);
> > >>> +
> > >>> + pr_crit("md/linear%s: Disk failure on %pg
> > >>> detected.\n"
> > >>> + "md/linear:%s: Cannot continue, failing
> > >>> array.\n",
> > >>> + md_name, rdev->bdev, md_name);
> > >> The second md_name is not needed.
> > > Could you elaborate here more? Do you want to skip device name in
> > > second message?
> >
> > Yes, we printed two md_name here, seems unnecessary.
> >
> I will merge errors to one message.
>
> >
> > >>> --- a/drivers/md/md.c
> > >>> +++ b/drivers/md/md.c
> > >>> @@ -7982,7 +7982,11 @@ void md_error(struct mddev *mddev, struct
> > >>> md_rdev *rdev)
> > >>> if (!mddev->pers || !mddev->pers->error_handler)
> > >>> return;
> > >>> - mddev->pers->error_handler(mddev,rdev);
> > >>> + mddev->pers->error_handler(mddev, rdev);
> > >>> +
> > >>> + if (!mddev->pers->sync_request)
> > >>> + return;
> > >> The above only valid for raid0 and linear, I guess it is fine if DM
> > >> don't create LV on top
> > >> of them. But the new checking deserves some comment above.
> > > Will do, could you propose comment?
> >
> > Or, just check if it is raid0 or linear directly instead of implies
> > level with
> > redundancy.
>
> Got it.
Could you please resend the patchset with feedback addressed? I can
apply the newer version and force push to md-next.
Thanks,
Song
