* Re: [PATCH] Forbid merging with partially assembled IMSM array [not found] <84A53BEA6EAC69439B7E311E9B17A76F07918BDA@IRSMSX105.ger.corp.intel.com> @ 2014-06-02 2:19 ` NeilBrown 2014-06-03 9:03 ` Baldysiak, Pawel 0 siblings, 1 reply; 10+ messages in thread From: NeilBrown @ 2014-06-02 2:19 UTC (permalink / raw) To: Baldysiak, Pawel; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur [-- Attachment #1: Type: text/plain, Size: 4046 bytes --] On Fri, 30 May 2014 13:11:43 +0000 "Baldysiak, Pawel" <pawel.baldysiak@intel.com> wrote: > Changes introduced in commit: > 0431869cec4c673309d9aa30a2df4b778bc0bd24 > enabled adding devices to partially assembled array. > It causes assemble process to override checking controller of device. > > For example: > If ones created IMSM array will be stopped, and some disks will be > attached to different controller, mdadm will allow to fully assemble this > array as one md device (due to marge one part to another). > > This patch resolve this problem by forbidding merge operation > on arrays with IMSM metadata. Why do you think this is a good thing? You seem to be saying "You cannot access that data because I don't like which controllers you have attached it to". My thought is that you should provide access to data whenever possible. Is there some important situation where the current behaviour will cause real problems? Maybe a warning might be appropriate, but failure doesn't seem sensible... NeilBrown > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> > Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > > --- > 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] Forbid merging with partially assembled IMSM array 2014-06-02 2:19 ` [PATCH] Forbid merging with partially assembled IMSM array NeilBrown @ 2014-06-03 9:03 ` Baldysiak, Pawel 2014-06-04 2:30 ` NeilBrown 0 siblings, 1 reply; 10+ messages in thread From: Baldysiak, Pawel @ 2014-06-03 9:03 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur >On Monday, June 02, 2014 4:20 AM NeilBrown [neilb@suse.de] wrote: > On Fri, 30 May 2014 13:11:43 +0000 "Baldysiak, Pawel" > <pawel.baldysiak@intel.com> wrote: > > > Changes introduced in commit: > > 0431869cec4c673309d9aa30a2df4b778bc0bd24 > > enabled adding devices to partially assembled array. > > It causes assemble process to override checking controller of device. > > > > For example: > > If ones created IMSM array will be stopped, and some disks will be > > attached to different controller, mdadm will allow to fully assemble > > this array as one md device (due to marge one part to another). > > > > This patch resolve this problem by forbidding merge operation on > > arrays with IMSM metadata. > > Why do you think this is a good thing? > You seem to be saying "You cannot access that data because I don't like > which controllers you have attached it to". > > My thought is that you should provide access to data whenever possible. > > Is there some important situation where the current behaviour will cause > real problems? > > Maybe a warning might be appropriate, but failure doesn't seem sensible... > > NeilBrown > Hi Neil, Sorry for all this mess with white-space corruption. Intel platforms have separate OpROMs for each controller. OpROMs manage SW RAIDs at startup and don't support arrays spanned across different controllers. In this case array will become degraded or failed after reboot anyway, because each OpROM will overwrite metadata on disks based on disks attached to specific controller. User should never be able to assemble IMSM array with disks under different controllers by default. You can always use "--force" to override this check. Pawel Baldysiak > > > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> > > Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > > > > --- > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Forbid merging with partially assembled IMSM array 2014-06-03 9:03 ` Baldysiak, Pawel @ 2014-06-04 2:30 ` NeilBrown 2014-06-04 9:25 ` Baldysiak, Pawel 0 siblings, 1 reply; 10+ messages in thread From: NeilBrown @ 2014-06-04 2:30 UTC (permalink / raw) To: Baldysiak, Pawel; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur [-- Attachment #1: Type: text/plain, Size: 5685 bytes --] On Tue, 3 Jun 2014 09:03:16 +0000 "Baldysiak, Pawel" <pawel.baldysiak@intel.com> wrote: > >On Monday, June 02, 2014 4:20 AM NeilBrown [neilb@suse.de] wrote: > > On Fri, 30 May 2014 13:11:43 +0000 "Baldysiak, Pawel" > > <pawel.baldysiak@intel.com> wrote: > > > > > Changes introduced in commit: > > > 0431869cec4c673309d9aa30a2df4b778bc0bd24 > > > enabled adding devices to partially assembled array. > > > It causes assemble process to override checking controller of device. > > > > > > For example: > > > If ones created IMSM array will be stopped, and some disks will be > > > attached to different controller, mdadm will allow to fully assemble > > > this array as one md device (due to marge one part to another). > > > > > > This patch resolve this problem by forbidding merge operation on > > > arrays with IMSM metadata. > > > > Why do you think this is a good thing? > > You seem to be saying "You cannot access that data because I don't like > > which controllers you have attached it to". > > > > My thought is that you should provide access to data whenever possible. > > > > Is there some important situation where the current behaviour will cause > > real problems? > > > > Maybe a warning might be appropriate, but failure doesn't seem sensible... > > > > NeilBrown > > > Hi Neil, > > Sorry for all this mess with white-space corruption. > > Intel platforms have separate OpROMs for each controller. > OpROMs manage SW RAIDs at startup and don't support arrays spanned across different controllers. > In this case array will become degraded or failed after reboot anyway, because each OpROM will overwrite metadata on disks based on disks attached to specific controller. If this is true, then surely we will never get into the situation where the same metadata is seen on different controller: the OpROM will have 'fixed' it already. > User should never be able to assemble IMSM array with disks under different controllers by default. I ask again: "Is there some important situation where the current behaviour will cause real problems?" If not, I don't feel that the change is justified. NeilBrown > You can always use "--force" to override this check. > > Pawel Baldysiak > > > > > > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> > > > Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > > > > > > --- > > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] Forbid merging with partially assembled IMSM array 2014-06-04 2:30 ` NeilBrown @ 2014-06-04 9:25 ` Baldysiak, Pawel 2014-06-05 6:27 ` NeilBrown 0 siblings, 1 reply; 10+ messages in thread From: Baldysiak, Pawel @ 2014-06-04 9:25 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur > On Wednesday, June 04, 2014 4:31 AM NeilBrown [neilb@suse.de] wrote: > Subject: Re: [PATCH] Forbid merging with partially assembled IMSM array > > On Tue, 3 Jun 2014 09:03:16 +0000 "Baldysiak, Pawel" > <pawel.baldysiak@intel.com> wrote: > > > >On Monday, June 02, 2014 4:20 AM NeilBrown [neilb@suse.de] wrote: > > > On Fri, 30 May 2014 13:11:43 +0000 "Baldysiak, Pawel" > > > <pawel.baldysiak@intel.com> wrote: > > > > > > > Changes introduced in commit: > > > > 0431869cec4c673309d9aa30a2df4b778bc0bd24 > > > > enabled adding devices to partially assembled array. > > > > It causes assemble process to override checking controller of device. > > > > > > > > For example: > > > > If ones created IMSM array will be stopped, and some disks will be > > > > attached to different controller, mdadm will allow to fully > > > > assemble this array as one md device (due to marge one part to > another). > > > > > > > > This patch resolve this problem by forbidding merge operation on > > > > arrays with IMSM metadata. > > > > > > Why do you think this is a good thing? > > > You seem to be saying "You cannot access that data because I don't > > > like which controllers you have attached it to". > > > > > > My thought is that you should provide access to data whenever possible. > > > > > > Is there some important situation where the current behaviour will > > > cause real problems? > > > > > > Maybe a warning might be appropriate, but failure doesn't seem > sensible... > > > > > > NeilBrown > > > > > Hi Neil, > > > > Sorry for all this mess with white-space corruption. > > > > Intel platforms have separate OpROMs for each controller. > > OpROMs manage SW RAIDs at startup and don't support arrays spanned > across different controllers. > > In this case array will become degraded or failed after reboot anyway, > because each OpROM will overwrite metadata on disks based on disks > attached to specific controller. > > If this is true, then surely we will never get into the situation where the same > metadata is seen on different controller: the OpROM will have 'fixed' it > already. > > > User should never be able to assemble IMSM array with disks under > different controllers by default. > > I ask again: > "Is there some important situation where the current behaviour will cause > real problems?" > > If not, I don't feel that the change is justified. > > NeilBrown 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"... Thanks Pawel Baldysiak > > > You can always use "--force" to override this check. > > > > Pawel Baldysiak > > > > > > > > > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> > > > > Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > > > > > > > > --- > > > > 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@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Forbid merging with partially assembled IMSM array 2014-06-04 9:25 ` Baldysiak, Pawel @ 2014-06-05 6:27 ` NeilBrown 2014-06-13 9:48 ` Baldysiak, Pawel 0 siblings, 1 reply; 10+ messages in thread From: NeilBrown @ 2014-06-05 6:27 UTC (permalink / raw) To: Baldysiak, Pawel; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur [-- Attachment #1: Type: text/plain, Size: 5727 bytes --] On Wed, 4 Jun 2014 09:25:47 +0000 "Baldysiak, Pawel" <pawel.baldysiak@intel.com> 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@intel.com> > > > > > Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > > > > > > > > > > --- > > > > > 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@vger.kernel.org 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] Forbid merging with partially assembled IMSM array 2014-06-05 6:27 ` NeilBrown @ 2014-06-13 9:48 ` Baldysiak, Pawel 2014-06-25 4:15 ` NeilBrown 0 siblings, 1 reply; 10+ messages in thread From: Baldysiak, Pawel @ 2014-06-13 9:48 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur >On Thursday, June 05, 2014 8:28 AM NeilBrown [neilb@suse.de] wrote: > To: Baldysiak, Pawel > Cc: linux-raid@vger.kernel.org; Paszkiewicz, Artur > Subject: Re: [PATCH] Forbid merging with partially assembled IMSM >array > > On Wed, 4 Jun 2014 09:25:47 +0000 "Baldysiak, Pawel" > <pawel.baldysiak@intel.com> 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 > Hi Neil, Here is the patch that adds warning message. Thanks Pawel Baldysiak From a0270724eb42e24a74508f5b0ec35afc64f17cf7 Mon Sep 17 00:00:00 2001 From: Pawel Baldysiak <pawel.baldysiak@intel.com> Date: Fri, 13 Jun 2014 10:55:19 +0200 Subject: [PATCH 1/1] IMSM: Add warning message when assemble spanned container Due to several changes in code assemble with disks spanned between different controllers can be obtained in some cases. After IMSM container will be assembled, check HBA of disks, and print proper warning if mismatch is detected. Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> --- Assemble.c | 28 ++++++++++++++++++++++++++++ platform-intel.h | 1 + 2 files changed, 29 insertions(+) diff --git a/Assemble.c b/Assemble.c index 63e09ac..3542b16 100644 --- a/Assemble.c +++ b/Assemble.c @@ -23,6 +23,7 @@ */ #include "mdadm.h" +#include "platform-intel.h" #include <ctype.h> static int name_matches(char *found, char *required, char *homehost) @@ -1001,6 +1002,33 @@ static int start_array(int mdfd, content->array.raid_disks); fprintf(stderr, "\n"); } + if (st->ss == &super_imsm && !check_env("IMSM_NO_PLATFORM")) { + struct sys_dev *idev; + char *hba_path = NULL; + char *dev_path = devt_to_devpath(makedev(devices->i.disk.major, + devices->i.disk.minor)); + + for (idev = find_intel_devices(); idev; idev = idev->next) { + if (strstr(dev_path, idev->path)) { + hba_path = idev->path; + break; + } + } + free(dev_path); + + if (hba_path) { + for (i = 1; (unsigned)i < okcnt+sparecnt; i++) { + if (!devt_attached_to_hba(makedev(devices[i].i.disk.major, + devices[i].i.disk.minor), hba_path)) { + pr_err("WARNING - IMSM container assembled with disks " + "under different HBAs!\n" + " This operation is not supported " + "and can lead to data loss.\n"); + break; + } + } + } + } st->ss->free_super(st); sysfs_uevent(content, "change"); if (err_ok && okcnt < (unsigned)content->array.raid_disks) diff --git a/platform-intel.h b/platform-intel.h index bcd84b7..999a601 100644 --- a/platform-intel.h +++ b/platform-intel.h @@ -203,6 +203,7 @@ struct sys_dev *find_driver_devices(const char *bus, const char *driver); struct sys_dev *find_intel_devices(void); const struct imsm_orom *find_imsm_capability(enum sys_dev_type hba_id); const struct imsm_orom *find_imsm_orom(void); +int devt_attached_to_hba(dev_t dev, const char *hba_path); int disk_attached_to_hba(int fd, const char *hba_path); char *devt_to_devpath(dev_t dev); int path_attached_to_hba(const char *disk_path, const char *hba_path); -- 1.9.3 > > > > > Thanks > > Pawel Baldysiak > > > > > > > > > You can always use "--force" to override this check. > > > > > > > > Pawel Baldysiak > > > > > > > > > > > > > > > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> > > > > > > Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > > > > > > > > > > > > --- > > > > > > 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@vger.kernel.org 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@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Forbid merging with partially assembled IMSM array 2014-06-13 9:48 ` Baldysiak, Pawel @ 2014-06-25 4:15 ` NeilBrown 2014-06-30 12:22 ` Baldysiak, Pawel 0 siblings, 1 reply; 10+ messages in thread From: NeilBrown @ 2014-06-25 4:15 UTC (permalink / raw) To: Baldysiak, Pawel; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur [-- Attachment #1: Type: text/plain, Size: 6048 bytes --] On Fri, 13 Jun 2014 09:48:16 +0000 "Baldysiak, Pawel" <pawel.baldysiak@intel.com> wrote: > >On Thursday, June 05, 2014 8:28 AM NeilBrown [neilb@suse.de] wrote: > > To: Baldysiak, Pawel > > Cc: linux-raid@vger.kernel.org; Paszkiewicz, Artur > > Subject: Re: [PATCH] Forbid merging with partially assembled IMSM > >array > > > > On Wed, 4 Jun 2014 09:25:47 +0000 "Baldysiak, Pawel" > > <pawel.baldysiak@intel.com> 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 > > > > Hi Neil, > > Here is the patch that adds warning message. > > Thanks > Pawel Baldysiak > > >From a0270724eb42e24a74508f5b0ec35afc64f17cf7 Mon Sep 17 00:00:00 2001 > From: Pawel Baldysiak <pawel.baldysiak@intel.com> > Date: Fri, 13 Jun 2014 10:55:19 +0200 > Subject: [PATCH 1/1] IMSM: Add warning message when assemble spanned container > > Due to several changes in code assemble with disks > spanned between different controllers can be obtained > in some cases. After IMSM container will be assembled, check HBA of > disks, and print proper warning if mismatch is detected. > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> > Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > --- > Assemble.c | 28 ++++++++++++++++++++++++++++ > platform-intel.h | 1 + > 2 files changed, 29 insertions(+) > > diff --git a/Assemble.c b/Assemble.c > index 63e09ac..3542b16 100644 > --- a/Assemble.c > +++ b/Assemble.c > @@ -23,6 +23,7 @@ > */ > > #include "mdadm.h" > +#include "platform-intel.h" > #include <ctype.h> > > static int name_matches(char *found, char *required, char *homehost) > @@ -1001,6 +1002,33 @@ static int start_array(int mdfd, > content->array.raid_disks); > fprintf(stderr, "\n"); > } > + if (st->ss == &super_imsm && !check_env("IMSM_NO_PLATFORM")) { > + struct sys_dev *idev; > + char *hba_path = NULL; > + char *dev_path = devt_to_devpath(makedev(devices->i.disk.major, > + devices->i.disk.minor)); > + > + for (idev = find_intel_devices(); idev; idev = idev->next) { > + if (strstr(dev_path, idev->path)) { > + hba_path = idev->path; > + break; > + } > + } > + free(dev_path); > + > + if (hba_path) { > + for (i = 1; (unsigned)i < okcnt+sparecnt; i++) { > + if (!devt_attached_to_hba(makedev(devices[i].i.disk.major, > + devices[i].i.disk.minor), hba_path)) { > + pr_err("WARNING - IMSM container assembled with disks " > + "under different HBAs!\n" > + " This operation is not supported " > + "and can lead to data loss.\n"); > + break; > + } > + } > + } > + } > st->ss->free_super(st); > sysfs_uevent(content, "change"); > if (err_ok && okcnt < (unsigned)content->array.raid_disks) > diff --git a/platform-intel.h b/platform-intel.h > index bcd84b7..999a601 100644 > --- a/platform-intel.h > +++ b/platform-intel.h > @@ -203,6 +203,7 @@ struct sys_dev *find_driver_devices(const char *bus, const char *driver); > struct sys_dev *find_intel_devices(void); > const struct imsm_orom *find_imsm_capability(enum sys_dev_type hba_id); > const struct imsm_orom *find_imsm_orom(void); > +int devt_attached_to_hba(dev_t dev, const char *hba_path); > int disk_attached_to_hba(int fd, const char *hba_path); > char *devt_to_devpath(dev_t dev); > int path_attached_to_hba(const char *disk_path, const char *hba_path); Could you please: 1/ move the code to super-intel.c and add a new superswitch method called "validate_array" or something like that. You can't pass the 'devices' array in as only Assemble.c knows about that, so filling the ->links in the mdinfo structure and pass the devices in as a list. 2/ Don't use a signed variable when you really want an unsigned variable! ('i' in the above code) 3/ Don't break strings to fit 80 columns. I know we have done that in the past, but I think it is more trouble than it is worth. Do break the string after "\n", but not elsewhere. Then I'll apply it. Thanks, NeilBrown [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] Forbid merging with partially assembled IMSM array 2014-06-25 4:15 ` NeilBrown @ 2014-06-30 12:22 ` Baldysiak, Pawel 2014-07-08 2:00 ` NeilBrown 0 siblings, 1 reply; 10+ messages in thread From: Baldysiak, Pawel @ 2014-06-30 12:22 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur > On Wednesday, June 25, 2014 6:16 AM NeilBrown wrote: > To: Baldysiak, Pawel > Cc: linux-raid@vger.kernel.org; Paszkiewicz, Artur > Subject: Re: [PATCH] Forbid merging with partially assembled IMSM array > (...) > Could you please: > > 1/ move the code to super-intel.c and add a new superswitch method called > "validate_array" or something like that. > You can't pass the 'devices' array in as only Assemble.c knows about that, > so filling the ->links in the mdinfo structure and pass the devices in as a > list. > > 2/ Don't use a signed variable when you really want an unsigned variable! > ('i' in the above code) > 3/ Don't break strings to fit 80 columns. I know we have done that in the > past, but I think it is more trouble than it is worth. Do break the > string after "\n", but not elsewhere. > > Then I'll apply it. > > Thanks, > NeilBrown Hi Neil Below is the patch with changes. Thanks Pawel Baldysiak From 7c18b28b7df82f35863674f3674b2a7e6a3f4040 Mon Sep 17 00:00:00 2001 From: Pawel Baldysiak <pawel.baldysiak@intel.com> Date: Mon, 30 Jun 2014 14:15:27 +0200 Subject: [PATCH 1/1] IMSM: Add warning message when assemble spanned container Due to several changes in code assemble with disks spanned between different controllers can be obtained in some cases. After IMSM container will be assembled, check HBA of disks, and print proper warning if mismatch is detected. Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> --- Assemble.c | 16 ++++++++++++++++ mdadm.h | 3 +++ platform-intel.h | 1 + super-intel.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+) diff --git a/Assemble.c b/Assemble.c index 63e09ac..aca28be 100644 --- a/Assemble.c +++ b/Assemble.c @@ -1001,6 +1001,22 @@ static int start_array(int mdfd, content->array.raid_disks); fprintf(stderr, "\n"); } + + if (st->ss->validate_container) { + struct mdinfo *devices_list; + struct mdinfo *info_devices = xmalloc(sizeof(struct mdinfo)*(okcnt+sparecnt)); + unsigned int count; + devices_list = NULL; + for (count = 0; count < okcnt+sparecnt; count++) { + info_devices[count] = devices[count].i; + info_devices[count].next = devices_list; + devices_list = &info_devices[count]; + } + if (st->ss->validate_container(devices_list)) + pr_err("Mismatch detected!\n"); + free(info_devices); + } + st->ss->free_super(st); sysfs_uevent(content, "change"); if (err_ok && okcnt < (unsigned)content->array.raid_disks) diff --git a/mdadm.h b/mdadm.h index 1734cfd..f60866c 100644 --- a/mdadm.h +++ b/mdadm.h @@ -965,6 +965,9 @@ extern struct superswitch { /* for external backup area */ int (*recover_backup)(struct supertype *st, struct mdinfo *info); + /* validate container after assemble */ + int (*validate_container)(struct mdinfo *info); + int swapuuid; /* true if uuid is bigending rather than hostendian */ int external; const char *name; /* canonical metadata name */ diff --git a/platform-intel.h b/platform-intel.h index bcd84b7..8226be3 100644 --- a/platform-intel.h +++ b/platform-intel.h @@ -204,6 +204,7 @@ struct sys_dev *find_intel_devices(void); const struct imsm_orom *find_imsm_capability(enum sys_dev_type hba_id); const struct imsm_orom *find_imsm_orom(void); int disk_attached_to_hba(int fd, const char *hba_path); +int devt_attached_to_hba(dev_t dev, const char *hba_path); char *devt_to_devpath(dev_t dev); int path_attached_to_hba(const char *disk_path, const char *hba_path); const char *get_sys_dev_type(enum sys_dev_type); diff --git a/super-intel.c b/super-intel.c index 9dd807a..a4d1e17 100644 --- a/super-intel.c +++ b/super-intel.c @@ -10484,6 +10484,48 @@ abort: return ret_val; } + +/******************************************************************************* + * Function: validate_container_imsm + * Description: This routine validates container after assemble, + * eg. if devices in container are under the same controller. + * + * Parameters: + * info : linked list with info about devices used in array + * Returns: + * 1 : HBA mismatch + * 0 : Success + ******************************************************************************/ +int validate_container_imsm(struct mdinfo *info) +{ + if (!check_env("IMSM_NO_PLATFORM")) { + struct sys_dev *idev; + struct mdinfo *dev; + char *hba_path = NULL; + char *dev_path = devt_to_devpath(makedev(info->disk.major, + info->disk.minor)); + + for (idev = find_intel_devices(); idev; idev = idev->next) { + if (strstr(dev_path, idev->path)) { + hba_path = idev->path; + break; + } + } + free(dev_path); + + if (hba_path) { + for (dev = info->next; dev; dev = dev->next) { + if (!devt_attached_to_hba(makedev(dev->disk.major, + dev->disk.minor), hba_path)) { + pr_err("WARNING - IMSM container assembled with disks under different HBAs!\n" + " This operation is not supported and can lead to data loss.\n"); + return 1; + } + } + } + } + return 0; +} #endif /* MDASSEMBLE */ struct superswitch super_imsm = { @@ -10527,6 +10569,7 @@ struct superswitch super_imsm = { .free_super = free_super_imsm, .match_metadata_desc = match_metadata_desc_imsm, .container_content = container_content_imsm, + .validate_container = validate_container_imsm, .external = 1, .name = "imsm", -- 1.9.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Forbid merging with partially assembled IMSM array 2014-06-30 12:22 ` Baldysiak, Pawel @ 2014-07-08 2:00 ` NeilBrown 0 siblings, 0 replies; 10+ messages in thread From: NeilBrown @ 2014-07-08 2:00 UTC (permalink / raw) To: Baldysiak, Pawel; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur [-- Attachment #1: Type: text/plain, Size: 6193 bytes --] On Mon, 30 Jun 2014 12:22:22 +0000 "Baldysiak, Pawel" <pawel.baldysiak@intel.com> wrote: > > On Wednesday, June 25, 2014 6:16 AM NeilBrown wrote: > > To: Baldysiak, Pawel > > Cc: linux-raid@vger.kernel.org; Paszkiewicz, Artur > > Subject: Re: [PATCH] Forbid merging with partially assembled IMSM array > > (...) > > Could you please: > > > > 1/ move the code to super-intel.c and add a new superswitch method called > > "validate_array" or something like that. > > You can't pass the 'devices' array in as only Assemble.c knows about that, > > so filling the ->links in the mdinfo structure and pass the devices in as a > > list. > > > > 2/ Don't use a signed variable when you really want an unsigned variable! > > ('i' in the above code) > > 3/ Don't break strings to fit 80 columns. I know we have done that in the > > past, but I think it is more trouble than it is worth. Do break the > > string after "\n", but not elsewhere. > > > > Then I'll apply it. > > > > Thanks, > > NeilBrown > > Hi Neil > Below is the patch with changes. > Thanks > Pawel Baldysiak Thanks. I've applied your patch. (though I generally prefer patches as separate emails - they are easier to apply that way). Thanks, NeilBrown > > >From 7c18b28b7df82f35863674f3674b2a7e6a3f4040 Mon Sep 17 00:00:00 2001 > From: Pawel Baldysiak <pawel.baldysiak@intel.com> > Date: Mon, 30 Jun 2014 14:15:27 +0200 > Subject: [PATCH 1/1] IMSM: Add warning message when assemble spanned container > > Due to several changes in code assemble with disks > spanned between different controllers can be obtained > in some cases. After IMSM container will be assembled, check HBA of > disks, and print proper warning if mismatch is detected. > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> > Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > --- > Assemble.c | 16 ++++++++++++++++ > mdadm.h | 3 +++ > platform-intel.h | 1 + > super-intel.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 63 insertions(+) > > diff --git a/Assemble.c b/Assemble.c > index 63e09ac..aca28be 100644 > --- a/Assemble.c > +++ b/Assemble.c > @@ -1001,6 +1001,22 @@ static int start_array(int mdfd, > content->array.raid_disks); > fprintf(stderr, "\n"); > } > + > + if (st->ss->validate_container) { > + struct mdinfo *devices_list; > + struct mdinfo *info_devices = xmalloc(sizeof(struct mdinfo)*(okcnt+sparecnt)); > + unsigned int count; > + devices_list = NULL; > + for (count = 0; count < okcnt+sparecnt; count++) { > + info_devices[count] = devices[count].i; > + info_devices[count].next = devices_list; > + devices_list = &info_devices[count]; > + } > + if (st->ss->validate_container(devices_list)) > + pr_err("Mismatch detected!\n"); > + free(info_devices); > + } > + > st->ss->free_super(st); > sysfs_uevent(content, "change"); > if (err_ok && okcnt < (unsigned)content->array.raid_disks) > diff --git a/mdadm.h b/mdadm.h > index 1734cfd..f60866c 100644 > --- a/mdadm.h > +++ b/mdadm.h > @@ -965,6 +965,9 @@ extern struct superswitch { > /* for external backup area */ > int (*recover_backup)(struct supertype *st, struct mdinfo *info); > > + /* validate container after assemble */ > + int (*validate_container)(struct mdinfo *info); > + > int swapuuid; /* true if uuid is bigending rather than hostendian */ > int external; > const char *name; /* canonical metadata name */ > diff --git a/platform-intel.h b/platform-intel.h > index bcd84b7..8226be3 100644 > --- a/platform-intel.h > +++ b/platform-intel.h > @@ -204,6 +204,7 @@ struct sys_dev *find_intel_devices(void); > const struct imsm_orom *find_imsm_capability(enum sys_dev_type hba_id); > const struct imsm_orom *find_imsm_orom(void); > int disk_attached_to_hba(int fd, const char *hba_path); > +int devt_attached_to_hba(dev_t dev, const char *hba_path); > char *devt_to_devpath(dev_t dev); > int path_attached_to_hba(const char *disk_path, const char *hba_path); > const char *get_sys_dev_type(enum sys_dev_type); > diff --git a/super-intel.c b/super-intel.c > index 9dd807a..a4d1e17 100644 > --- a/super-intel.c > +++ b/super-intel.c > @@ -10484,6 +10484,48 @@ abort: > > return ret_val; > } > + > +/******************************************************************************* > + * Function: validate_container_imsm > + * Description: This routine validates container after assemble, > + * eg. if devices in container are under the same controller. > + * > + * Parameters: > + * info : linked list with info about devices used in array > + * Returns: > + * 1 : HBA mismatch > + * 0 : Success > + ******************************************************************************/ > +int validate_container_imsm(struct mdinfo *info) > +{ > + if (!check_env("IMSM_NO_PLATFORM")) { > + struct sys_dev *idev; > + struct mdinfo *dev; > + char *hba_path = NULL; > + char *dev_path = devt_to_devpath(makedev(info->disk.major, > + info->disk.minor)); > + > + for (idev = find_intel_devices(); idev; idev = idev->next) { > + if (strstr(dev_path, idev->path)) { > + hba_path = idev->path; > + break; > + } > + } > + free(dev_path); > + > + if (hba_path) { > + for (dev = info->next; dev; dev = dev->next) { > + if (!devt_attached_to_hba(makedev(dev->disk.major, > + dev->disk.minor), hba_path)) { > + pr_err("WARNING - IMSM container assembled with disks under different HBAs!\n" > + " This operation is not supported and can lead to data loss.\n"); > + return 1; > + } > + } > + } > + } > + return 0; > +} > #endif /* MDASSEMBLE */ > > struct superswitch super_imsm = { > @@ -10527,6 +10569,7 @@ struct superswitch super_imsm = { > .free_super = free_super_imsm, > .match_metadata_desc = match_metadata_desc_imsm, > .container_content = container_content_imsm, > + .validate_container = validate_container_imsm, > > .external = 1, > .name = "imsm", [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Forbid merging with partially assembled IMSM array @ 2014-05-30 14:36 Baldysiak, Pawel 0 siblings, 0 replies; 10+ messages in thread From: Baldysiak, Pawel @ 2014-05-30 14:36 UTC (permalink / raw) To: neilb@suse.de; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur Changes introduced in commit: 0431869cec4c673309d9aa30a2df4b778bc0bd24 enabled adding devices to partially assembled array. It causes assemble process to override checking controller of device. For example: If ones created IMSM array will be stopped, and some disks will be attached to different controller, mdadm will allow to fully assemble this array as one md device (due to marge one part to another). This patch resolve this problem by forbidding merge operation on arrays with IMSM metadata. Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com> Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> --- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-07-08 2:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <84A53BEA6EAC69439B7E311E9B17A76F07918BDA@IRSMSX105.ger.corp.intel.com>
2014-06-02 2:19 ` [PATCH] Forbid merging with partially assembled IMSM array NeilBrown
2014-06-03 9:03 ` Baldysiak, Pawel
2014-06-04 2:30 ` NeilBrown
2014-06-04 9:25 ` Baldysiak, Pawel
2014-06-05 6:27 ` NeilBrown
2014-06-13 9:48 ` Baldysiak, Pawel
2014-06-25 4:15 ` NeilBrown
2014-06-30 12:22 ` Baldysiak, Pawel
2014-07-08 2:00 ` NeilBrown
2014-05-30 14:36 Baldysiak, Pawel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).