| Message ID | 8255ED565A0EAA4A960625E27D49DBF24AC9EB61@SACEXCMBX04-PRD.hq.netapp.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On Fri, Mar 28, 2014 at 09:01:14PM +0000, Stewart, Sean wrote: > When a system is booted to the SAN, a condition can occur where one > user friendly name is given to a disk during boot, but multipathd tries > to allocate a different one after boot. If the second alias is already > used by another device, multipathd can't rename it. Multipathd then has > incorrect information about the alias/wwid relationships, which can > result in paths being added to the wrong map. This should only happen if the initramfs and root file system have inconsistent multipath configurations (either multipath.conf or bindings / wwids file mismatched). That's not really a valid configuration for the system to be in and leads to the type of problems you describe. > This patch works around this problem by first trying to use the alias > already bound to a device during boot. If the bindings file has that > alias bound to a different device, it'll auto generate a new alias to > rename it to. To be honest I'd prefer to see this cause an error. These types of configurations currently run the risk of silent data corruption - I'd much rather deal with a system that refuses to boot due to an out of date initramfs image than one that quietly remaps paths in unexpected ways. Regards, Bryn. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Fri, 2014-04-11 at 17:03 +0100, Bryn M. Reeves wrote: > On Fri, Mar 28, 2014 at 09:01:14PM +0000, Stewart, Sean wrote: > > When a system is booted to the SAN, a condition can occur where one > > user friendly name is given to a disk during boot, but multipathd tries > > to allocate a different one after boot. If the second alias is already > > used by another device, multipathd can't rename it. Multipathd then has > > incorrect information about the alias/wwid relationships, which can > > result in paths being added to the wrong map. > > This should only happen if the initramfs and root file system have > inconsistent multipath configurations (either multipath.conf or bindings > / wwids file mismatched). That's not really a valid configuration for > the system to be in and leads to the type of problems you describe. That is true that it only happens if they are out of sync. We tried remaking the initramfs to fix the problem, but it didn't help. > > > This patch works around this problem by first trying to use the alias > > already bound to a device during boot. If the bindings file has that > > alias bound to a different device, it'll auto generate a new alias to > > rename it to. > > To be honest I'd prefer to see this cause an error. These types of > configurations currently run the risk of silent data corruption - I'd > much rather deal with a system that refuses to boot due to an out of > date initramfs image than one that quietly remaps paths in unexpected > ways. The issue, though, is that the system does not refuse to boot. In the case we saw, it booted anyway, our QA engineer ran a test, and it ended with a data corruption. A user could perform a fresh installation, map new luns, reboot, and without any way of realizing it have essentially a ticking time bomb on their hands, ready to go off as soon as there's a blip in the SAN. Here's some sample output from when I recreated the problem on my local system: I started with a fresh sanboot installation to verify I see the problem: Bindings file: mpatha 360080e50001b08100000184e5320d0f3 mpathb 360080e50001b0810000018705320f03d mpathc 360080e50001b0810000004a051ef5f71 mpathd 360080e50001b076d0000cd8251ef5fe5 mpathe 360080e50001b076d0000cd8651ef5ff0 mpathf 360080e50001b076d0000cd8851ef5ff6 Multipath -ll [root@localhost ~]# multipath -ll | grep mpath mpathe (360080e50001b076d0000cd8251ef5fe5) dm-3 LSI ,INF-01-00 mpathd (360080e50001b076d0000cd8651ef5ff0) dm-2 LSI ,INF-01-00 mpathc (360080e50001b076d0000cd8851ef5ff6) dm-1 LSI ,INF-01-00 mpathb (360080e50001b0810000004a051ef5f71) dm-0 LSI ,INF-01-00 mpathf (360080e50001b0810000018705320f03d) dm-4 LSI ,INF-01-00 During bootup, it assigned 360080e50001b076d0000cd8251ef5fe5 to be mpathe. After the system came up, it tried to assign it to be mpathd. When it tried to rename it, it failed because there's already an mpathd. Here's an example of how that can show up in the logs: Mar 18 15:52:15 localhost multipathd: 360080e50001b0810000004a051ef5f71: unable to rename mpathe to mpathb (mpathb is used by 360080e50001b076d0000cd8251ef5fe5) Mar 18 15:52:15 localhost multipathd: 360080e50001b076d0000cd8251ef5fe5: unable to rename mpathb to mpathc (mpathc is used by 360080e50001b076d0000cd8651ef5ff0) Mar 18 15:52:15 localhost multipathd: 360080e50001b076d0000cd8651ef5ff0: unable to rename mpathc to mpathd (mpathd is used by 360080e50001b076d0000cd8851ef5ff6) Mar 18 15:52:15 localhost multipathd: 360080e50001b076d0000cd8851ef5ff6: unable to rename mpathd to mpathe (mpathe is used by 360080e50001b0810000004a051ef5f71) So, if I delete a path from mpathe, then scan it again, it ends up incorrectly with mpathd (because that's the alias multipathd thinks is assigned to that WWID). The maps end up looking like this: mpathe (360080e50001b076d0000cd8251ef5fe5) dm-3 LSI ,INF-01-00 size=2.0G features='4 queue_if_no_path pg_init_retries 50 retain_attached_hw_handle' hwhandler='1 rdac' wp=rw |-+- policy='service-time 0' prio=14 status=active | |- 1:0:1:2 sdc 8:32 active ready running | `- 4:0:1:2 sdm 8:192 active ready running `-+- policy='service-time 0' prio=9 status=enabled |- #:#:#:# - #:# active faulty running `- 4:0:2:2 sdr 65:16 active ready running mpathd (360080e50001b076d0000cd8651ef5ff0) dm-2 LSI ,INF-01-00 size=2.0G features='4 queue_if_no_path pg_init_retries 50 retain_attached_hw_handle' hwhandler='1 rdac' wp=rw |-+- policy='service-time 0' prio=14 status=active | |- 1:0:1:3 sdd 8:48 active ready running | |- 4:0:1:3 sdn 8:208 active ready running | |- 1:0:1:2 sdc 8:32 active ready running | `- 4:0:1:2 sdm 8:192 active ready running `-+- policy='service-time 0' prio=9 status=enabled |- 1:0:2:3 sdi 8:128 active ready running |- 4:0:2:3 sds 65:32 active ready running |- 4:0:2:2 sdr 65:16 active ready running `- 1:0:2:2 sdu 65:64 active ready running If a user wants a specific alias, they should still configure it through multipath.conf. It just prevents a potentially really bad condition that could occur, if the setting is enabled, and something as simple as adding new luns is done. Thanks, Sean > > Regards, > Bryn. > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Ping... Any additional comments or suggestions for this patch? Bumping in case it got lost in the backlog. :) On Fri, 2014-04-11 at 17:01 +0000, Stewart, Sean wrote: > On Fri, 2014-04-11 at 17:03 +0100, Bryn M. Reeves wrote: > > On Fri, Mar 28, 2014 at 09:01:14PM +0000, Stewart, Sean wrote: > > > When a system is booted to the SAN, a condition can occur where one > > > user friendly name is given to a disk during boot, but multipathd tries > > > to allocate a different one after boot. If the second alias is already > > > used by another device, multipathd can't rename it. Multipathd then has > > > incorrect information about the alias/wwid relationships, which can > > > result in paths being added to the wrong map. > > > > This should only happen if the initramfs and root file system have > > inconsistent multipath configurations (either multipath.conf or bindings > > / wwids file mismatched). That's not really a valid configuration for > > the system to be in and leads to the type of problems you describe. > > That is true that it only happens if they are out of sync. We tried > remaking the initramfs to fix the problem, but it didn't help. > > > > > This patch works around this problem by first trying to use the alias > > > already bound to a device during boot. If the bindings file has that > > > alias bound to a different device, it'll auto generate a new alias to > > > rename it to. > > > > To be honest I'd prefer to see this cause an error. These types of > > configurations currently run the risk of silent data corruption - I'd > > much rather deal with a system that refuses to boot due to an out of > > date initramfs image than one that quietly remaps paths in unexpected > > ways. > > The issue, though, is that the system does not refuse to boot. In the > case we saw, it booted anyway, our QA engineer ran a test, and it ended > with a data corruption. A user could perform a fresh installation, > map > new luns, reboot, and without any way of realizing it have essentially a > ticking time bomb on their hands, ready to go off as soon as there's a > blip in the SAN. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Ben, I'd need your ack on this one. Best regards, Christophe Varoqui On Thu, May 15, 2014 at 9:21 PM, Stewart, Sean <Sean.Stewart@netapp.com>wrote: > Ping... Any additional comments or suggestions for this patch? > Bumping in case it got lost in the backlog. :) > > On Fri, 2014-04-11 at 17:01 +0000, Stewart, Sean wrote: > > On Fri, 2014-04-11 at 17:03 +0100, Bryn M. Reeves wrote: > > > On Fri, Mar 28, 2014 at 09:01:14PM +0000, Stewart, Sean wrote: > > > > When a system is booted to the SAN, a condition can occur where one > > > > user friendly name is given to a disk during boot, but multipathd > tries > > > > to allocate a different one after boot. If the second alias is > already > > > > used by another device, multipathd can't rename it. Multipathd then > has > > > > incorrect information about the alias/wwid relationships, which can > > > > result in paths being added to the wrong map. > > > > > > This should only happen if the initramfs and root file system have > > > inconsistent multipath configurations (either multipath.conf or > bindings > > > / wwids file mismatched). That's not really a valid configuration for > > > the system to be in and leads to the type of problems you describe. > > > > That is true that it only happens if they are out of sync. We tried > > remaking the initramfs to fix the problem, but it didn't help. > > > > > > > This patch works around this problem by first trying to use the alias > > > > already bound to a device during boot. If the bindings file has that > > > > alias bound to a different device, it'll auto generate a new alias to > > > > rename it to. > > > > > > To be honest I'd prefer to see this cause an error. These types of > > > configurations currently run the risk of silent data corruption - I'd > > > much rather deal with a system that refuses to boot due to an out of > > > date initramfs image than one that quietly remaps paths in unexpected > > > ways. > > > > The issue, though, is that the system does not refuse to boot. In the > > case we saw, it booted anyway, our QA engineer ran a test, and it ended > > with a data corruption. A user could perform a fresh installation, > > map > > new luns, reboot, and without any way of realizing it have essentially a > > ticking time bomb on their hands, ready to go off as soon as there's a > > blip in the SAN. > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, May 15, 2014 at 11:45:40PM +0200, Christophe Varoqui wrote: > Ben, > I'd need your ack on this one. > Best regards, > Christophe Varoqui Sorry I dropped the ball on this one. I'm o.k. with this patch. The biggest issue I have with it has nothing to do with its correctness, but with rlookup_wwid()'s use of scan_device. Previously, the only scan_device call always failed. Now scan every device name, but we don't ever get anything out of it. First off, if we find a match, we will never use the id. Second, if we don't find a match we return the id that of the alias we were looking for, but if we do find a match we return the next id after the one we were looking for (which is completely pointless). It seems like we could just make rlookup_wwid() return success or failure, and then call scan_device() from use_existing_alias() if we need to, and take out a bunch of pointless work that rlookup_wwid() is doing. -Ben > > On Thu, May 15, 2014 at 9:21 PM, Stewart, Sean > <[1]Sean.Stewart@netapp.com> wrote: > > Ping... Any additional comments or suggestions for this patch? > Bumping in case it got lost in the backlog. :) > On Fri, 2014-04-11 at 17:01 +0000, Stewart, Sean wrote: > > On Fri, 2014-04-11 at 17:03 +0100, Bryn M. Reeves wrote: > > > On Fri, Mar 28, 2014 at 09:01:14PM +0000, Stewart, Sean wrote: > > > > When a system is booted to the SAN, a condition can occur where > one > > > > user friendly name is given to a disk during boot, but multipathd > tries > > > > to allocate a different one after boot. If the second alias is > already > > > > used by another device, multipathd can't rename it. Multipathd > then has > > > > incorrect information about the alias/wwid relationships, which > can > > > > result in paths being added to the wrong map. > > > > > > This should only happen if the initramfs and root file system have > > > inconsistent multipath configurations (either multipath.conf or > bindings > > > / wwids file mismatched). That's not really a valid configuration > for > > > the system to be in and leads to the type of problems you describe. > > > > That is true that it only happens if they are out of sync. We tried > > remaking the initramfs to fix the problem, but it didn't help. > > > > > > > This patch works around this problem by first trying to use the > alias > > > > already bound to a device during boot. If the bindings file has > that > > > > alias bound to a different device, it'll auto generate a new alias > to > > > > rename it to. > > > > > > To be honest I'd prefer to see this cause an error. These types of > > > configurations currently run the risk of silent data corruption - > I'd > > > much rather deal with a system that refuses to boot due to an out of > > > date initramfs image than one that quietly remaps paths in > unexpected > > > ways. > > > > The issue, though, is that the system does not refuse to boot. In the > > case we saw, it booted anyway, our QA engineer ran a test, and it > ended > > with a data corruption. A user could perform a fresh installation, > > map > > new luns, reboot, and without any way of realizing it have essentially > a > > ticking time bomb on their hands, ready to go off as soon as there's a > > blip in the SAN. > > References > > Visible links > 1. mailto:Sean.Stewart@netapp.com -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Ok, I guess those changes can come incrementally over this patch then. Applied. Christophe Varoqui www.opensvc.com On Mon, Jun 9, 2014 at 10:22 PM, Benjamin Marzinski <bmarzins@redhat.com> wrote: > On Thu, May 15, 2014 at 11:45:40PM +0200, Christophe Varoqui wrote: > > Ben, > > I'd need your ack on this one. > > Best regards, > > Christophe Varoqui > > Sorry I dropped the ball on this one. > > I'm o.k. with this patch. The biggest issue I have with it has nothing > to do with its correctness, but with rlookup_wwid()'s use of scan_device. > Previously, the only scan_device call always failed. Now scan every > device name, but we don't ever get anything out of it. First off, if we > find a match, we will never use the id. Second, if we don't find a match we > return the id that of the alias we were looking for, but if we do find a > match we return the next id after the one we were looking for (which is > completely pointless). > > It seems like we could just make rlookup_wwid() return success or failure, > and then call scan_device() from use_existing_alias() if we need to, and > take out a bunch of pointless work that rlookup_wwid() is doing. > > -Ben > > > > > On Thu, May 15, 2014 at 9:21 PM, Stewart, Sean > > <[1]Sean.Stewart@netapp.com> wrote: > > > > Ping... Any additional comments or suggestions for this patch? > > Bumping in case it got lost in the backlog. :) > > On Fri, 2014-04-11 at 17:01 +0000, Stewart, Sean wrote: > > > On Fri, 2014-04-11 at 17:03 +0100, Bryn M. Reeves wrote: > > > > On Fri, Mar 28, 2014 at 09:01:14PM +0000, Stewart, Sean wrote: > > > > > When a system is booted to the SAN, a condition can occur > where > > one > > > > > user friendly name is given to a disk during boot, but > multipathd > > tries > > > > > to allocate a different one after boot. If the second alias is > > already > > > > > used by another device, multipathd can't rename it. Multipathd > > then has > > > > > incorrect information about the alias/wwid relationships, > which > > can > > > > > result in paths being added to the wrong map. > > > > > > > > This should only happen if the initramfs and root file system > have > > > > inconsistent multipath configurations (either multipath.conf or > > bindings > > > > / wwids file mismatched). That's not really a valid > configuration > > for > > > > the system to be in and leads to the type of problems you > describe. > > > > > > That is true that it only happens if they are out of sync. We > tried > > > remaking the initramfs to fix the problem, but it didn't help. > > > > > > > > > This patch works around this problem by first trying to use > the > > alias > > > > > already bound to a device during boot. If the bindings file > has > > that > > > > > alias bound to a different device, it'll auto generate a new > alias > > to > > > > > rename it to. > > > > > > > > To be honest I'd prefer to see this cause an error. These types > of > > > > configurations currently run the risk of silent data corruption > - > > I'd > > > > much rather deal with a system that refuses to boot due to an > out of > > > > date initramfs image than one that quietly remaps paths in > > unexpected > > > > ways. > > > > > > The issue, though, is that the system does not refuse to boot. > In the > > > case we saw, it booted anyway, our QA engineer ran a test, and it > > ended > > > with a data corruption. A user could perform a fresh > installation, > > > map > > > new luns, reboot, and without any way of realizing it have > essentially > > a > > > ticking time bomb on their hands, ready to go off as soon as > there's a > > > blip in the SAN. > > > > References > > > > Visible links > > 1. mailto:Sean.Stewart@netapp.com > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/alias.c b/libmultipath/alias.c index ab15185..008396e 100644 --- a/libmultipath/alias.c +++ b/libmultipath/alias.c @@ -145,7 +145,7 @@ lookup_binding(FILE *f, char *map_wwid, char **map_alias, char *prefix) } static int -rlookup_binding(FILE *f, char *buff, char *map_alias) +rlookup_binding(FILE *f, char *buff, char *map_alias, char *prefix) { char line[LINE_MAX]; unsigned int line_nr = 0; @@ -164,7 +164,7 @@ rlookup_binding(FILE *f, char *buff, char *map_alias) alias = strtok(line, " \t"); if (!alias) /* blank line */ continue; - curr_id = scan_devname(alias, NULL); /* TBD: Why this call? */ + curr_id = scan_devname(alias, prefix); if (curr_id >= id) id = curr_id + 1; wwid = strtok(NULL, " \t"); @@ -188,6 +188,11 @@ rlookup_binding(FILE *f, char *buff, char *map_alias) } } condlog(3, "No matching alias [%s] in bindings file.", map_alias); + + /* Get the theoretical id for this map alias. + * Used by use_existing_alias + */ + id = scan_devname(map_alias, prefix); return id; } @@ -237,6 +242,59 @@ allocate_binding(int fd, char *wwid, int id, char *prefix) } char * +use_existing_alias (char *wwid, char *file, char *alias_old, + char *prefix, int bindings_read_only) +{ + char *alias = NULL; + int id = 0; + int fd, can_write; + char buff[WWID_SIZE]; + FILE *f; + + fd = open_file(file, &can_write, BINDINGS_FILE_HEADER); + if (fd < 0) + return NULL; + + f = fdopen(fd, "r"); + if (!f) { + condlog(0, "cannot fdopen on bindings file descriptor"); + close(fd); + return NULL; + } + /* lookup the binding. if it exsists, the wwid will be in buff + * either way, id contains the id for the alias + */ + id = rlookup_binding(f , buff, alias_old, prefix); + if (id < 0) + goto out; + + if (strlen(buff) > 0) { + /* if buff is our wwid, it's already + * allocated correctly + */ + if (strcmp(buff, wwid) == 0) + alias = STRDUP(alias_old); + else { + alias = NULL; + condlog(0, "alias %s already bound to wwid %s, cannot reuse", + alias_old, buff); + } + goto out; + } + + /* allocate the existing alias in the bindings file */ + if (can_write && id && !bindings_read_only) { + alias = allocate_binding(fd, wwid, id, prefix); + condlog(0, "Allocated existing binding [%s] for WWID [%s]", + alias, wwid); + } + +out: + fclose(f); + return alias; +} + +char * get_user_friendly_alias(char *wwid, char *file, char *prefix, int bindings_read_only) { @@ -305,7 +363,7 @@ get_user_friendly_wwid(char *alias, char *buff, char *file) return -1; } - rlookup_binding(f, buff, alias); + rlookup_binding(f, buff, alias, NULL); if (!strlen(buff)) { fclose(f); return -1; diff --git a/libmultipath/alias.h b/libmultipath/alias.h index 8ddd0b5..9cb3e8f 100644 --- a/libmultipath/alias.h +++ b/libmultipath/alias.h @@ -10,3 +10,5 @@ char *get_user_friendly_alias(char *wwid, char *file, char *prefix, int bindings_readonly); int get_user_friendly_wwid(char *alias, char *buff, char *file); +char *use_existing_alias (char *wwid, char *file, char *alias_old, + char *prefix, int bindings_read_only); diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c index 7b7944d..2859e3e 100644 --- a/libmultipath/propsel.c +++ b/libmultipath/propsel.c @@ -253,19 +253,31 @@ want_user_friendly_names(struct multipath * mp) extern int select_alias (struct multipath * mp) { - if (mp->mpe && mp->mpe->alias) + if (mp->mpe && mp->mpe->alias) { mp->alias = STRDUP(mp->mpe->alias); - else { - mp->alias = NULL; - if (want_user_friendly_names(mp)) { - select_alias_prefix(mp); - mp->alias = get_user_friendly_alias(mp->wwid, - conf->bindings_file, mp->alias_prefix, conf->bindings_read_only); - } - if (mp->alias == NULL) - mp->alias = STRDUP(mp->wwid); + goto out; } + mp->alias = NULL; + if (!want_user_friendly_names(mp)) + goto out; + + select_alias_prefix(mp); + + if (strlen(mp->alias_old) > 0) { + mp->alias = use_existing_alias(mp->wwid, conf->bindings_file, + mp->alias_old, mp->alias_prefix, + conf->bindings_read_only); + memset (mp->alias_old, 0, WWID_SIZE); + } + + if (mp->alias == NULL) + mp->alias = get_user_friendly_alias(mp->wwid, + conf->bindings_file, mp->alias_prefix, conf->bindings_read_only); +out: + if (mp->alias == NULL) + mp->alias = STRDUP(mp->wwid); + return mp->alias ? 0 : 1; } diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c index 76c7e02..8cdbe3d 100644 --- a/libmultipath/structs_vec.c +++ b/libmultipath/structs_vec.c @@ -403,6 +403,20 @@ out: return NULL; } +static void +find_existing_alias (struct multipath * mpp, + struct vectors *vecs) +{ + struct multipath * mp; + int i; + + vector_foreach_slot (vecs->mpvec, mp, i) + if (strcmp(mp->wwid, mpp->wwid) == 0) { + strncpy(mpp->alias_old, mp->alias, WWID_SIZE); + return; + } +} + extern struct multipath * add_map_with_path (struct vectors * vecs, struct path * pp, int add_vec) @@ -416,6 +430,7 @@ add_map_with_path (struct vectors * vecs, mpp->hwe = pp->hwe; strcpy(mpp->wwid, pp->wwid); + find_existing_alias(mpp, vecs); if (select_alias(mpp)) goto out; mpp->size = pp->size;