On Wed, 4 Jun 2014 09:25:47 +0000 "Baldysiak, Pawel"
<pawel.baldysiak@xxxxxxxxx> wrote:
> Hi Neil
>
> There are several cases where not forbidding this operation can cause problems.
> For example, if user will create IMSM array at one platform, then stop it, detach devices,
> and hotplug them to another platform (mixed between two different controllers) -
> everything will be looking good (array will be incrementally assembled without any errors) until reboot.
> After that, user will be confused why array is degraded/failed and will be at risk of losing data.
> Also, creation of IMSM array under different controllers is forbidden, so there is no point in allowing that it in assemble process.
>
> If you really think this is no justification - I will prepare a patch that will print warning,
> but in my opinion this operation should be allowed only with "--force"...
>
I came across a situation once where someone was trying to crash-dump onto an
IMSM array. 'crashdump' does some sort of kexec thing and runs a very
minimal kernel/installation which just does crash dumps.
It didn't work because mdadm couldn't find the OpROM in that context and so
refused the assemble the array.
I really don't like that sort of thing happening.
If the OpROM is going to destroy your data (or whatever it does) when you
plug different devices into different controllers, then that is the OpROM's
problem, not mine. I just want to make sure people can get at their data
in as many situations as possible.
A warning is probably OK, though I wonder if anyone would ever see it.
However the code would need improvement.
Doing a strcmp for "imsm" is not acceptable.
I'm not even sure I understand what they rest of the code is trying to do.
It doesn't seem to make reference to separate OpROMs....
NeilBrown
> Thanks
> Pawel Baldysiak
>
> >
> > > You can always use "--force" to override this check.
> > >
> > > Pawel Baldysiak
> > >
> > > > >
> > > > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@xxxxxxxxx>
> > > > > Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx>
> > > > >
> > > > > ---
> > > > > Assemble.c | 23 +++++++++++++++++++++++
> > > > > 1 file changed, 23 insertions(+)
> > > > >
> > > > > diff --git a/Assemble.c b/Assemble.c index a57d384..9fb65e5 100644
> > > > > --- a/Assemble.c
> > > > > +++ b/Assemble.c
> > > > > @@ -1329,6 +1329,29 @@ try_again:
> > > > > return 1;
> > > > > }
> > > > > for (dv = pre_exist->devs; dv; dv =
> > > > > dv->next) {
> > > > > + if
> > > > > + (strncmp(mp->metadata, "imsm", 4) == 0 && !c-
> > > > >force) {
> > > > > + int dfd;
> > > > > + char dn[20];
> > > > > + struct supertype *st2;
> > > > > + sprintf(dn, "%d:%d", dv->disk.major,
> > > > > + dv->disk.minor);
> > > > > + st2 = dup_super(st);
> > > > > + dfd = dev_open(dn, O_RDONLY);
> > > > > + if
> > > > > + ((dfd > 0) && (st2->ss->load_super(st2,
> > > > dfd, NULL) ||
> > > > > + (st->ss->compare_super(st, st2) !=
> > 0))) {
> > > > > +
> > > > > + pr_err("IMSM metadata
> > > > mismatch!\n");
> > > > > + pr_err("Aborting...\n");
> > > > > + close(dfd);
> > > > > + } else {
> > > > > +
> > > > > + pr_err("IMSM Array already
> > > > partially assembled!\n");
> > > > > + pr_err("Aborting...\n");
> > > > > + }
> > > > > + if (st2) {
> > > > > + st2->ss->free_super(st2);
> > > > > + free(st2);
> > > > > + }
> > > > > + return 1;
> > > > > + }
> > > > > /* We want to add this device to our list,
> > > > > * but it could already be there if "mdadm -I"
> > > > > * started *after* we checked for O_EXCL.
> > > > > --
> > > > > 1.9.0
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-raid"
> > > in the body of a message to majordomo@xxxxxxxxxxxxxxx More
> > majordomo
> > > info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
