* [dm-devel] [PATCH 0/2] Handle remapped LUNs better @ 2021-02-09 5:19 Benjamin Marzinski 2021-02-09 5:19 ` [dm-devel] [PATCH 1/2] libmultipath: fix use-after-free in uev_add_path Benjamin Marzinski 2021-02-09 5:19 ` [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid Benjamin Marzinski 0 siblings, 2 replies; 13+ messages in thread From: Benjamin Marzinski @ 2021-02-09 5:19 UTC (permalink / raw) To: Christophe Varoqui; +Cc: device-mapper development, Chongyun Wu, Martin Wilck This patchset adds a new config option, recheck_wwid_time, to help deal with devices getting remapped. It's based on Chongyun's patch, but instead of always checking if the LUN is remapped, users can set how many seconds the LUN must be down before it gets rechecked, or disable this checking entirely, since it is simply there to keep users from shooting themselves in the foot. Setting the value to 0 makes this always recheck when a path is restored. Unlike Chongyun's patch, it doesn't issue a remove uevent. I'm not actually sure what the purpose of the remove uevent was, since it doesn't cause the path to be removed, and multipath already removed it. Instead, it works like Martin's uev_update_path code, and re-adds the path after it removes it. There is one issue I noticed here. While udev will update the WWID in the database when it gets a new uevent, sysfs will still record the original WWID, so if multipathd ever fails back to using sysfs for the WWID, it will see the wrong value. Another possible check that I didn't include is this patchset is to have multipath look at the LUN number whenever it adds a new path to a multipath device. If the LUN number doesn't match the existing LUN numbers, all the old paths should have their wwids checked as soon as possible (immediately, if the path is up), since mismatching LUN numbers is a red flag that something has gone wrong. Benjamin Marzinski (2): libmultipath: fix use-after-free in uev_add_path multipathd: add recheck_wwid_time option to verify the path wwid libmultipath/config.c | 1 + libmultipath/config.h | 1 + libmultipath/configure.c | 4 +- libmultipath/configure.h | 2 + libmultipath/defaults.h | 1 + libmultipath/dict.c | 36 ++++++++++ libmultipath/libmultipath.version | 6 ++ libmultipath/structs.h | 10 +++ multipath/multipath.conf.5 | 18 +++++ multipathd/cli_handlers.c | 9 +++ multipathd/main.c | 107 +++++++++++++++++++++++++++--- multipathd/main.h | 2 + 12 files changed, 187 insertions(+), 10 deletions(-) -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [dm-devel] [PATCH 1/2] libmultipath: fix use-after-free in uev_add_path 2021-02-09 5:19 [dm-devel] [PATCH 0/2] Handle remapped LUNs better Benjamin Marzinski @ 2021-02-09 5:19 ` Benjamin Marzinski 2021-02-09 20:57 ` Martin Wilck 2021-02-09 5:19 ` [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid Benjamin Marzinski 1 sibling, 1 reply; 13+ messages in thread From: Benjamin Marzinski @ 2021-02-09 5:19 UTC (permalink / raw) To: Christophe Varoqui; +Cc: device-mapper development, Chongyun Wu, Martin Wilck if ev_remove_path() returns success the path has very likely been deleted. However, if pathinfo() returned something besides PATHINFO_OK, but ev_remove_path() succeeded, uev_add_path() was still accessing the the path afterwards, which would likely cause a use-after-free error. Insted, uev_add_path() should only continue to access the path if ev_remove_path() didn't succeed. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- multipathd/main.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/multipathd/main.c b/multipathd/main.c index 425492a9..19679848 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -890,13 +890,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map) */ pp->mpp = prev_mpp; ret = ev_remove_path(pp, vecs, true); - if (r == PATHINFO_OK && !ret) - /* - * Path successfully freed, move on to - * "new path" code path below - */ - pp = NULL; - else { + if (ret != 0) { /* * Failure in ev_remove_path will keep * path in pathvec in INIT_REMOVED state @@ -907,7 +901,12 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map) dm_fail_path(pp->mpp->alias, pp->dev_t); condlog(1, "%s: failed to re-add path still mapped in %s", pp->dev, pp->mpp->alias); - } + } else if (r == PATHINFO_OK) + /* + * Path successfully freed, move on to + * "new path" code path below + */ + pp = NULL; } else if (r == PATHINFO_SKIPPED) { condlog(3, "%s: remove blacklisted path", uev->kernel); -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 1/2] libmultipath: fix use-after-free in uev_add_path 2021-02-09 5:19 ` [dm-devel] [PATCH 1/2] libmultipath: fix use-after-free in uev_add_path Benjamin Marzinski @ 2021-02-09 20:57 ` Martin Wilck 0 siblings, 0 replies; 13+ messages in thread From: Martin Wilck @ 2021-02-09 20:57 UTC (permalink / raw) To: bmarzins, christophe.varoqui; +Cc: dm-devel, wucy11 On Mon, 2021-02-08 at 23:19 -0600, Benjamin Marzinski wrote: > if ev_remove_path() returns success the path has very likely been > deleted. However, if pathinfo() returned something besides > PATHINFO_OK, > but ev_remove_path() succeeded, uev_add_path() was still accessing > the > the path afterwards, which would likely cause a use-after-free error. > Insted, uev_add_path() should only continue to access the path if > ev_remove_path() didn't succeed. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Reviewed-by: Martin Wilck <mwilck@suse.com> -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-09 5:19 [dm-devel] [PATCH 0/2] Handle remapped LUNs better Benjamin Marzinski 2021-02-09 5:19 ` [dm-devel] [PATCH 1/2] libmultipath: fix use-after-free in uev_add_path Benjamin Marzinski @ 2021-02-09 5:19 ` Benjamin Marzinski 2021-02-09 22:19 ` Martin Wilck 1 sibling, 1 reply; 13+ messages in thread From: Benjamin Marzinski @ 2021-02-09 5:19 UTC (permalink / raw) To: Christophe Varoqui; +Cc: device-mapper development, Chongyun Wu, Martin Wilck There are cases where the wwid of a path changes due to LUN remapping without triggering uevent for the changed path. Multipathd has no method for trying to catch these cases, and corruption has resulted because of it. In order to have a better chance at catching these cases, multipath now has a recheck_wwid_time option, which can either be set to "off" or a number of seconds. If a path is failed for equal to or greater than the configured number of seconds, multipathd will recheck its wwid before restoring it, when the path checker sees that it has come back up. If multipathd notices that a path's wwid has changed it will remove and re-add the path, just like the existing wwid checking code for change events does. In cases where the no uevent occurs, both the udev database entry and sysfs will have the old wwid, so the only way to get a current wwid is to ask the device directly. Currently multipath only has code to directly get the wwid for scsi devices, so this option only effects scsi devices. Also, since it's possible the the udev wwid won't match the wwid from get_vpd_sgio(), if multipathd doesn't initially see the two values matching for a device, it will disable this option for that device. If recheck_wwid_time is not turned off, multipathd will also automatically recheck the wwid whenever an existing path gets a add event, or is manually re-added with cli_add_path(). Co-developed-by: Chongyun Wu <wucy11@chinatelecom.cn> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/config.c | 1 + libmultipath/config.h | 1 + libmultipath/configure.c | 4 +- libmultipath/configure.h | 2 + libmultipath/defaults.h | 1 + libmultipath/dict.c | 36 ++++++++++++ libmultipath/libmultipath.version | 6 ++ libmultipath/structs.h | 10 ++++ multipath/multipath.conf.5 | 18 ++++++ multipathd/cli_handlers.c | 9 +++ multipathd/main.c | 92 +++++++++++++++++++++++++++++++ multipathd/main.h | 2 + 12 files changed, 180 insertions(+), 2 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index be310159..c79ebbeb 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -867,6 +867,7 @@ int _init_config (const char *file, struct config *conf) conf->remove_retries = 0; conf->ghost_delay = DEFAULT_GHOST_DELAY; conf->all_tg_pt = DEFAULT_ALL_TG_PT; + conf->recheck_wwid_time = DEFAULT_RECHECK_WWID; /* * preload default hwtable */ diff --git a/libmultipath/config.h b/libmultipath/config.h index 9ce37f16..02ae2407 100644 --- a/libmultipath/config.h +++ b/libmultipath/config.h @@ -187,6 +187,7 @@ struct config { int marginal_pathgroups; int skip_delegate; unsigned int sequence_nr; + int recheck_wwid_time; char * multipath_dir; char * selector; diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 598efe05..6ca1f4bb 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -527,8 +527,8 @@ get_udev_for_mpp(const struct multipath *mpp) return udd; } -static void trigger_partitions_udev_change(struct udev_device *dev, - const char *action, int len) +void trigger_partitions_udev_change(struct udev_device *dev, + const char *action, int len) { struct udev_enumerate *part_enum; struct udev_list_entry *item; diff --git a/libmultipath/configure.h b/libmultipath/configure.h index 6b23ccbb..70cf77a3 100644 --- a/libmultipath/configure.h +++ b/libmultipath/configure.h @@ -58,3 +58,5 @@ int get_refwwid (enum mpath_cmds cmd, const char *dev, enum devtypes dev_type, vector pathvec, char **wwid); struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type); void trigger_paths_udev_change(struct multipath *mpp, bool is_mpath); +void trigger_partitions_udev_change(struct udev_device *dev, const char *action, + int len); diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h index 947ba467..d24b69a0 100644 --- a/libmultipath/defaults.h +++ b/libmultipath/defaults.h @@ -55,6 +55,7 @@ #define DEFAULT_ALL_TG_PT ALL_TG_PT_OFF /* Enable no foreign libraries by default */ #define DEFAULT_ENABLE_FOREIGN "NONE" +#define DEFAULT_RECHECK_WWID RECHECK_WWID_OFF #define CHECKINT_UNDEF UINT_MAX #define DEFAULT_CHECKINT 5 diff --git a/libmultipath/dict.c b/libmultipath/dict.c index bab96146..85782ea3 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -1401,6 +1401,41 @@ declare_hw_snprint(all_tg_pt, print_yes_no_undef) declare_def_handler(marginal_pathgroups, set_yes_no) declare_def_snprint(marginal_pathgroups, print_yes_no) +static int +set_recheck_wwid_time(vector strvec, void *ptr) +{ + int *int_ptr = (int *)ptr; + char * buff; + + buff = set_value(strvec); + if (!buff) + return 1; + + if (!strcmp(buff, "no") || !strcmp(buff, "off")) + *int_ptr = RECHECK_WWID_OFF; + else if (sscanf(buff, "%d", int_ptr) != 1 || *int_ptr < 0) + *int_ptr = DEFAULT_RECHECK_WWID; + + FREE(buff); + return 0; +} + +int +print_recheck_wwid_time(char * buff, int len, long v) +{ + switch(v) { + case RECHECK_WWID_OFF: + return snprintf(buff, len, "\"off\""); + default: + return snprintf(buff, len, "%li", v); + } +} + + + +declare_def_handler(recheck_wwid_time, set_recheck_wwid_time) +declare_def_snprint(recheck_wwid_time, print_recheck_wwid_time) + static int def_uxsock_timeout_handler(struct config *conf, vector strvec) { @@ -1819,6 +1854,7 @@ init_keywords(vector keywords) install_keyword("enable_foreign", &def_enable_foreign_handler, &snprint_def_enable_foreign); install_keyword("marginal_pathgroups", &def_marginal_pathgroups_handler, &snprint_def_marginal_pathgroups); + install_keyword("recheck_wwid_time", &def_recheck_wwid_time_handler, &snprint_def_recheck_wwid_time); __deprecated install_keyword("default_selector", &def_selector_handler, NULL); __deprecated install_keyword("default_path_grouping_policy", &def_pgpolicy_handler, NULL); __deprecated install_keyword("default_uid_attribute", &def_uid_attribute_handler, NULL); diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version index 2228f4ec..e9b4608f 100644 --- a/libmultipath/libmultipath.version +++ b/libmultipath/libmultipath.version @@ -280,3 +280,9 @@ LIBMULTIPATH_4.4.0 { global: get_next_string; } LIBMULTIPATH_4.3.0; + +LIBMULITIPATH_4.5.0 { +global: + get_vpd_sgio; + trigger_partitions_udev_change; +} LIBMULTIPATH_4.4.0; diff --git a/libmultipath/structs.h b/libmultipath/structs.h index d6ff6762..357fdc24 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -38,6 +38,7 @@ #define NO_PATH_RETRY_FAIL -1 #define NO_PATH_RETRY_QUEUE -2 +#define RECHECK_WWID_OFF -1 enum free_path_mode { KEEP_PATHS, @@ -242,6 +243,13 @@ enum eh_deadline_states { EH_DEADLINE_ZERO = UOZ_ZERO, }; + +enum allow_wwid_recheck_states { + ALLOW_WWID_RECHECK_UNSET, + ALLOW_WWID_RECHECK_OFF, + ALLOW_WWID_RECHECK_ON, +}; + struct vpd_vendor_page { int pg; const char *name; @@ -316,6 +324,8 @@ struct path { int find_multipaths_timeout; int marginal; int vpd_vendor_id; + int failed_time; + int allow_wwid_recheck; /* configlet pointers */ vector hwe; struct gen_path generic_path; diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index 8ef3a747..50409d57 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -1273,6 +1273,24 @@ The default is: \fB\(dqNONE\(dq\fR .RE . . +.TP +.B recheck_wwid_time +Sets the number of seconds that a path device can be failed for, before +multipathd rechecks its wwid when it is restored. If the wwid has changed, +the path is removed from the current multipath device, and re-added as a new +path. If set to \fIoff\fR these rechecks are disabled. If set to \fI0\fR, +multipath will always recheck the path's wwid whenever it is restored. If +rechecks are enabled, they will also happen if an \fIadd\fR uevent occurs +for an existing path, or an existing path is manually re-added. These rechecks +will only happen for scsi devices. \fBNote:\fR When the path is originally +added, if the path's configured wwid doesn't match the wwid retrieved directly +from the scsi device, rechecks will be disabled for the device. +.RS +.TP +The default is \fBoff\fR +.RE +. +. . .\" ---------------------------------------------------------------------------- diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c index 54635738..970d5e21 100644 --- a/multipathd/cli_handlers.c +++ b/multipathd/cli_handlers.c @@ -715,6 +715,15 @@ cli_add_path (void * v, char ** reply, int * len, void * data) pp = find_path_by_dev(vecs->pathvec, param); if (pp && pp->initialized != INIT_REMOVED) { condlog(2, "%s: path already in pathvec", param); + + if (pp->allow_wwid_recheck == ALLOW_WWID_RECHECK_ON && + check_path_wwid_change(pp)) { + condlog(0, "%s: wwid changed. Removing device", + pp->dev); + handle_path_wwid_change(pp, vecs); + return 1; + } + if (pp->mpp) return 0; } else if (pp) { diff --git a/multipathd/main.c b/multipathd/main.c index 19679848..17eef3a4 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -823,6 +823,59 @@ ev_remove_map (char * devname, char * alias, int minor, struct vectors * vecs) return flush_map(mpp, vecs, 0); } +void +handle_path_wwid_change(struct path *pp, struct vectors *vecs) +{ + const char *action = "add"; + if (!pp || !pp->udev) + return; + + sysfs_attr_set_value(pp->udev, "uevent", action, strlen(action)); + trigger_partitions_udev_change(pp->udev, action, strlen(action)); + if (ev_remove_path(pp, vecs, 1) != 0 && pp->mpp) { + pp->dmstate = PSTATE_FAILED; + dm_fail_path(pp->mpp->alias, pp->dev_t); + } +} + +bool +check_path_wwid_change(struct path *pp) +{ + char wwid[WWID_SIZE]; + int len = 0; + char *c; + + if (!strlen(pp->wwid) || pp->bus != SYSFS_BUS_SCSI) + return false; + + /* Get the real fresh device wwid by sgio. sysfs still has old + * data, so only get_vpd_sgio will work to get the new wwid */ + len = get_vpd_sgio(pp->fd, 0x83, 0, wwid, WWID_SIZE); + + if (len <= 0) { + condlog(2, "%s: failed to check wwid by sgio: len = %d", + pp->dev, len); + return false; + } + + /*Strip any trailing blanks */ + c = strchr(wwid, '\0'); + c--; + while (c && c >= wwid && *c == ' ') { + *c = '\0'; + c--; + } + condlog(4, "%s: Got wwid %s by sgio", pp->dev, wwid); + + if (strncmp(wwid, pp->wwid, WWID_SIZE)) { + condlog(0, "%s: wwid '%s' doesn't match wwid '%s' from device", + pp->dev, pp->wwid, wwid); + return true; + } + + return false; +} + static int uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map) { @@ -919,6 +972,21 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map) uev->kernel); ret = 1; } + } else if (pp->allow_wwid_recheck == ALLOW_WWID_RECHECK_ON && + check_path_wwid_change(pp)) { + condlog(2, "%s wwid change detected when processing add uevent. removing path", pp->dev); + /* + * don't use handle_path_wwid_change here, + * since that would just trigger another add + * uevent + */ + ret = ev_remove_path(pp, vecs, true); + if (ret == 0) + pp = NULL; + else if (pp->mpp) { + pp->dmstate = PSTATE_FAILED; + dm_fail_path(pp->mpp->alias, pp->dev_t); + } } } if (pp) @@ -2049,6 +2117,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) unsigned int checkint, max_checkint; struct config *conf; int marginal_pathgroups, marginal_changed = 0; + int recheck_wwid_time; int ret; if (((pp->initialized == INIT_OK || @@ -2066,6 +2135,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) checkint = conf->checkint; max_checkint = conf->max_checkint; marginal_pathgroups = conf->marginal_pathgroups; + recheck_wwid_time = conf->recheck_wwid_time; put_multipath_config(conf); if (pp->checkint == CHECKINT_UNDEF) { @@ -2197,6 +2267,26 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) return 0; set_no_path_retry(pp->mpp); + if (recheck_wwid_time != RECHECK_WWID_OFF && + (newstate == PATH_UP || newstate == PATH_GHOST)) { + if (pp->allow_wwid_recheck == ALLOW_WWID_RECHECK_UNSET) { + if (check_path_wwid_change(pp)) + pp->allow_wwid_recheck = ALLOW_WWID_RECHECK_OFF; + else + pp->allow_wwid_recheck = ALLOW_WWID_RECHECK_ON; + } else if (((pp->state != PATH_UP && pp->state != PATH_GHOST) || + pp->dmstate == PSTATE_FAILED) && + pp->failed_time >= recheck_wwid_time && + pp->allow_wwid_recheck == ALLOW_WWID_RECHECK_ON && + check_path_wwid_change(pp)) { + condlog(0, "%s: path wwid change detected. Removing", + pp->dev); + handle_path_wwid_change(pp, vecs); + return 0; + } + pp->failed_time = 0; + } + if ((newstate == PATH_UP || newstate == PATH_GHOST) && (san_path_check_enabled(pp->mpp) || marginal_path_check_enabled(pp->mpp))) { @@ -2330,6 +2420,8 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) if (newstate == PATH_DOWN) { int log_checker_err; + if (recheck_wwid_time != RECHECK_WWID_OFF) + pp->failed_time += pp->checkint; conf = get_multipath_config(); log_checker_err = conf->log_checker_err; put_multipath_config(conf); diff --git a/multipathd/main.h b/multipathd/main.h index 5abbe97b..ddd953f9 100644 --- a/multipathd/main.h +++ b/multipathd/main.h @@ -50,4 +50,6 @@ int update_multipath (struct vectors *vecs, char *mapname, int reset); int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs, int refresh); +void handle_path_wwid_change(struct path *pp, struct vectors *vecs); +bool check_path_wwid_change(struct path *pp); #endif /* MAIN_H */ -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-09 5:19 ` [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid Benjamin Marzinski @ 2021-02-09 22:19 ` Martin Wilck 2021-02-10 18:09 ` Benjamin Block ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Martin Wilck @ 2021-02-09 22:19 UTC (permalink / raw) To: bmarzins, christophe.varoqui; +Cc: dm-devel, wucy11 On Mon, 2021-02-08 at 23:19 -0600, Benjamin Marzinski wrote: > There are cases where the wwid of a path changes due to LUN remapping > without triggering uevent for the changed path. Multipathd has no > method > for trying to catch these cases, and corruption has resulted because > of > it. > > In order to have a better chance at catching these cases, multipath > now > has a recheck_wwid_time option, which can either be set to "off" or a > number of seconds. If a path is failed for equal to or greater than > the > configured number of seconds, multipathd will recheck its wwid before > restoring it, when the path checker sees that it has come back up. Can't the WWID change also happen without the path going offline, or at least without being offline long enough that multipathd would notice? > If > multipathd notices that a path's wwid has changed it will remove and > re-add the path, just like the existing wwid checking code for change > events does. In cases where the no uevent occurs, both the udev > database entry and sysfs will have the old wwid, so the only way to > get > a current wwid is to ask the device directly. sysfs is wrong too, really? In that case I fear triggering an uevent won't fix the situation. You need to force the kernel to rescan the device, otherwise udev will fetch the WWID from sysfs again, which still has the wrong ID... or what am I missing here? > > Currently multipath only > has code to directly get the wwid for scsi devices, so this option > only > effects scsi devices. Also, since it's possible the the udev wwid > won't > match the wwid from get_vpd_sgio(), if multipathd doesn't initially > see > the two values matching for a device, it will disable this option for > that device. > > If recheck_wwid_time is not turned off, multipathd will also > automatically recheck the wwid whenever an existing path gets a add > event, or is manually re-added with cli_add_path(). > > Co-developed-by: Chongyun Wu <wucy11@chinatelecom.cn> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> I am uncertain about this. We get one more configuration option that defaults to off and that only the truly inaugurated will understand and use. And even those will not know how to set the recheck time. Should it be 1s, 10, or 100? We already have too many of these options in multipath-tools. We shy away from giving users reasonable defaults, with the result that most people won't bother. I generally don't understand what the UP/DOWN state has to do with this. If the WWID can change without any events seen by either the kernel or user space, why would the path go down and up again? And even if so, why would it matter how long the device remained down? But foremost, do we really have to try to deal with configuration mistakes as blatant as this? What if a user sets the same WWID for different devices, or re-uses the same WWID on different storage servers? I already hesitated about the code I added myself for catching user errors in the WWIDs file, but this seems even more far-fetched. Please convince me. This said, I'd like to understand why there are no events in these cases. I'd have thought we'd at least get a UNIT ATTENTION (REPORTED LUNS DATA HAS CHANGED), which would have caused a uevent. If there was no UNIT ATTENTION, I'd blame the storage side. Maybe we need to monitor scsi_device uevents. Technical remarks below. > --- > libmultipath/config.c | 1 + > libmultipath/config.h | 1 + > libmultipath/configure.c | 4 +- > libmultipath/configure.h | 2 + > libmultipath/defaults.h | 1 + > libmultipath/dict.c | 36 ++++++++++++ > libmultipath/libmultipath.version | 6 ++ > libmultipath/structs.h | 10 ++++ > multipath/multipath.conf.5 | 18 ++++++ > multipathd/cli_handlers.c | 9 +++ > multipathd/main.c | 92 > +++++++++++++++++++++++++++++++ > multipathd/main.h | 2 + > 12 files changed, 180 insertions(+), 2 deletions(-) > . > .\" ---------------------------------------------------------------- > ------------ > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c > index 54635738..970d5e21 100644 > --- a/multipathd/cli_handlers.c > +++ b/multipathd/cli_handlers.c > @@ -715,6 +715,15 @@ cli_add_path (void * v, char ** reply, int * > len, void * data) > pp = find_path_by_dev(vecs->pathvec, param); > if (pp && pp->initialized != INIT_REMOVED) { > condlog(2, "%s: path already in pathvec", param); > + > + if (pp->allow_wwid_recheck == ALLOW_WWID_RECHECK_ON > && > + check_path_wwid_change(pp)) { > + condlog(0, "%s: wwid changed. Removing > device", > + pp->dev); > + handle_path_wwid_change(pp, vecs); > + return 1; > + } > + > if (pp->mpp) > return 0; > } else if (pp) { > diff --git a/multipathd/main.c b/multipathd/main.c > index 19679848..17eef3a4 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -823,6 +823,59 @@ ev_remove_map (char * devname, char * alias, int > minor, struct vectors * vecs) > return flush_map(mpp, vecs, 0); > } > > +void > +handle_path_wwid_change(struct path *pp, struct vectors *vecs) > +{ > + const char *action = "add"; > + if (!pp || !pp->udev) > + return; > + > + sysfs_attr_set_value(pp->udev, "uevent", action, > strlen(action)); > + trigger_partitions_udev_change(pp->udev, action, > strlen(action)); Nit: it looks a bit weird to use a const char * variable for "action" and a constant for "uevent". > + if (ev_remove_path(pp, vecs, 1) != 0 && pp->mpp) { > + pp->dmstate = PSTATE_FAILED; > + dm_fail_path(pp->mpp->alias, pp->dev_t); > + } I suggest taking a ref on pp->udev, calling ev_remove_path(), and triggering the uevent after that. > +} > + > +bool > +check_path_wwid_change(struct path *pp) > +{ > + char wwid[WWID_SIZE]; > + int len = 0; > + char *c; > + > + if (!strlen(pp->wwid) || pp->bus != SYSFS_BUS_SCSI) > + return false; Maybe you should look at uid_attribute here, to be consistent with has_uid_fallback()? > + > + /* Get the real fresh device wwid by sgio. sysfs still has > old > + * data, so only get_vpd_sgio will work to get the new wwid > */ > + len = get_vpd_sgio(pp->fd, 0x83, 0, wwid, WWID_SIZE); > + > + if (len <= 0) { > + condlog(2, "%s: failed to check wwid by sgio: len = > %d", > + pp->dev, len); > + return false; > + } > + > + /*Strip any trailing blanks */ > + c = strchr(wwid, '\0'); Quite unusual, why not use strlen() or strnlen()? > + c--; > + while (c && c >= wwid && *c == ' ') { > + *c = '\0'; > + c--; > + } Nit: You don't have to nullify every space. Just the first one. > + condlog(4, "%s: Got wwid %s by sgio", pp->dev, wwid); > + > + if (strncmp(wwid, pp->wwid, WWID_SIZE)) { > + condlog(0, "%s: wwid '%s' doesn't match wwid '%s' > from device", > + pp->dev, pp->wwid, wwid); > + return true; > + } > + > + return false; > +} > + > static int > uev_add_path (struct uevent *uev, struct vectors * vecs, int > need_do_map) > { > @@ -919,6 +972,21 @@ uev_add_path (struct uevent *uev, struct vectors > * vecs, int need_do_map) > uev->kernel); > ret = 1; > } > + } else if (pp->allow_wwid_recheck == > ALLOW_WWID_RECHECK_ON && > + check_path_wwid_change(pp)) { > + condlog(2, "%s wwid change detected when > processing add uevent. removing path", pp->dev); > + /* > + * don't use handle_path_wwid_change here, > + * since that would just trigger another add > + * uevent > + */ > + ret = ev_remove_path(pp, vecs, true); > + if (ret == 0) > + pp = NULL; > + else if (pp->mpp) { > + pp->dmstate = PSTATE_FAILED; > + dm_fail_path(pp->mpp->alias, pp- > >dev_t); > + } What's the purpose of this code? Are you handling your own artificial "add" event here, which you triggered before in handle_path_wwid_change()? Or are there real cases where the kernel would send an "add" event without sending a "remove" event first? > } > } > if (pp) > @@ -2049,6 +2117,7 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > unsigned int checkint, max_checkint; > struct config *conf; > int marginal_pathgroups, marginal_changed = 0; > + int recheck_wwid_time; > int ret; > > if (((pp->initialized == INIT_OK || > @@ -2066,6 +2135,7 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > checkint = conf->checkint; > max_checkint = conf->max_checkint; > marginal_pathgroups = conf->marginal_pathgroups; > + recheck_wwid_time = conf->recheck_wwid_time; > put_multipath_config(conf); > > if (pp->checkint == CHECKINT_UNDEF) { > @@ -2197,6 +2267,26 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > return 0; > set_no_path_retry(pp->mpp); > > + if (recheck_wwid_time != RECHECK_WWID_OFF && > + (newstate == PATH_UP || newstate == PATH_GHOST)) { > + if (pp->allow_wwid_recheck == > ALLOW_WWID_RECHECK_UNSET) { > + if (check_path_wwid_change(pp)) > + pp->allow_wwid_recheck = > ALLOW_WWID_RECHECK_OFF; > + else > + pp->allow_wwid_recheck = > ALLOW_WWID_RECHECK_ON; This is confusing. I pulled my hair over this code before I read your man page hunk: "When the path is originally added, if the path's configured wwid doesn't match the wwid retrieved directly from the scsi device, rechecks will be disabled for the device." So, I gather this is an alternative to the has_uid_fallback() logic mentioned above. It deserves at least a comment here. Please consider if using the same logic as has_uid_falback() isn't just as good as this. Btw, as we're already pretty much on corner-case ground here, if the path fails quickly after being detected, a WWID change could have occured already when it comes UP first, and this code is run for the first time. > + } else if (((pp->state != PATH_UP && pp->state != > PATH_GHOST) || > + pp->dmstate == PSTATE_FAILED) && > + pp->failed_time >= recheck_wwid_time && > + pp->allow_wwid_recheck == > ALLOW_WWID_RECHECK_ON && > + check_path_wwid_change(pp)) { > + condlog(0, "%s: path wwid change detected. > Removing", > + pp->dev); > + handle_path_wwid_change(pp, vecs); > + return 0; > + } > + pp->failed_time = 0; > + } > + > if ((newstate == PATH_UP || newstate == PATH_GHOST) && > (san_path_check_enabled(pp->mpp) || > marginal_path_check_enabled(pp->mpp))) { > @@ -2330,6 +2420,8 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > if (newstate == PATH_DOWN) { > int log_checker_err; > > + if (recheck_wwid_time != RECHECK_WWID_OFF) > + pp->failed_time += pp->checkint; > conf = get_multipath_config(); > log_checker_err = conf->log_checker_err; > put_multipath_config(conf); > diff --git a/multipathd/main.h b/multipathd/main.h > index 5abbe97b..ddd953f9 100644 > --- a/multipathd/main.h > +++ b/multipathd/main.h > @@ -50,4 +50,6 @@ int update_multipath (struct vectors *vecs, char > *mapname, int reset); > int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs, > int refresh); > > +void handle_path_wwid_change(struct path *pp, struct vectors *vecs); > +bool check_path_wwid_change(struct path *pp); > #endif /* MAIN_H */ -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-09 22:19 ` Martin Wilck @ 2021-02-10 18:09 ` Benjamin Block 2021-02-10 19:57 ` Martin Wilck 2021-02-11 11:25 ` Benjamin Block 2021-02-11 4:48 ` Benjamin Marzinski 2021-02-27 6:02 ` Benjamin Marzinski 2 siblings, 2 replies; 13+ messages in thread From: Benjamin Block @ 2021-02-10 18:09 UTC (permalink / raw) To: Martin Wilck; +Cc: dm-devel, wucy11 On Tue, Feb 09, 2021 at 10:19:45PM +0000, Martin Wilck wrote: > On Mon, 2021-02-08 at 23:19 -0600, Benjamin Marzinski wrote: > > There are cases where the wwid of a path changes due to LUN remapping > > without triggering uevent for the changed path. Multipathd has no > > method > > for trying to catch these cases, and corruption has resulted because > > of > > it. > > > > In order to have a better chance at catching these cases, multipath > > now > > has a recheck_wwid_time option, which can either be set to "off" or a > > number of seconds. If a path is failed for equal to or greater than > > the > > configured number of seconds, multipathd will recheck its wwid before > > restoring it, when the path checker sees that it has come back up. > > Can't the WWID change also happen without the path going offline, or > at least without being offline long enough that multipathd would > notice? > > > If > > multipathd notices that a path's wwid has changed it will remove and > > re-add the path, just like the existing wwid checking code for change > > events does. In cases where the no uevent occurs, both the udev > > database entry and sysfs will have the old wwid, so the only way to > > get > > a current wwid is to ask the device directly. > > sysfs is wrong too, really? In that case I fear triggering an uevent > won't fix the situation. You need to force the kernel to rescan the > device, otherwise udev will fetch the WWID from sysfs again, which > still has the wrong ID... or what am I missing here? > > > > Currently multipath only > > has code to directly get the wwid for scsi devices, so this option > > only > > effects scsi devices. Also, since it's possible the the udev wwid > > won't > > match the wwid from get_vpd_sgio(), if multipathd doesn't initially > > see > > the two values matching for a device, it will disable this option for > > that device. > > > > If recheck_wwid_time is not turned off, multipathd will also > > automatically recheck the wwid whenever an existing path gets a add > > event, or is manually re-added with cli_add_path(). > > > > Co-developed-by: Chongyun Wu <wucy11@chinatelecom.cn> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > I am uncertain about this. > > We get one more configuration option that defaults to off and that only > the truly inaugurated will understand and use. And even those will not > know how to set the recheck time. Should it be 1s, 10, or 100? We > already have too many of these options in multipath-tools. We shy away > from giving users reasonable defaults, with the result that most people > won't bother. > > I generally don't understand what the UP/DOWN state has to do with > this. If the WWID can change without any events seen by either the > kernel or user space, why would the path go down and up again? And even > if so, why would it matter how long the device remained down? > > But foremost, do we really have to try to deal with configuration > mistakes as blatant as this? What if a user sets the same WWID for > different devices, or re-uses the same WWID on different storage > servers? I already hesitated about the code I added myself for catching > user errors in the WWIDs file, but this seems even more far-fetched. > > Please convince me. > > This said, I'd like to understand why there are no events in these > cases. I'd have thought we'd at least get a UNIT ATTENTION (REPORTED > LUNS DATA HAS CHANGED), which would have caused a uevent. If there was > no UNIT ATTENTION, I'd blame the storage side. Yeah, just for reference, I saw this happening in practice when something with the LU mapping changed on IBM storage - IIRC I saw it with capacity changes. You end up in this code in the kernel: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_error.c?id=92bf22614b21a2706f4993b278017e437f7785b3#n416 And from there you ought to get an uevent for the sdev. The WWID in sysfs might still be wrong though AFAIK. The kernel seems to ignore the UA after it delivered the uevent. > > Maybe we need to monitor scsi_device uevents. > > Technical remarks below. > > -- Best Regards, Benjamin Block / Linux on IBM Z Kernel Development / IBM Systems IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy Vorsitz. AufsR.: Gregor Pillen / Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-10 18:09 ` Benjamin Block @ 2021-02-10 19:57 ` Martin Wilck 2021-02-11 11:25 ` Benjamin Block 1 sibling, 0 replies; 13+ messages in thread From: Martin Wilck @ 2021-02-10 19:57 UTC (permalink / raw) To: bblock; +Cc: dm-devel, wucy11 On Wed, 2021-02-10 at 19:09 +0100, Benjamin Block wrote: > > Yeah, just for reference, I saw this happening in practice when > something with the LU mapping changed on IBM storage - IIRC I saw it > with capacity changes. You end up in this code in the kernel: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_error.c?id=92bf22614b21a2706f4993b278017e437f7785b3#n416 > > And from there you ought to get an uevent for the sdev. > > The WWID in sysfs might still be wrong though AFAIK. The kernel seems > to > ignore the UA after it delivered the uevent. Right. We could trigger a device rescan if such an event was received. Could be done from udev rules, or even from multipathd itself. Thanks, Martin -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-10 18:09 ` Benjamin Block 2021-02-10 19:57 ` Martin Wilck @ 2021-02-11 11:25 ` Benjamin Block 1 sibling, 0 replies; 13+ messages in thread From: Benjamin Block @ 2021-02-11 11:25 UTC (permalink / raw) To: dm-devel On Wed, Feb 10, 2021 at 07:09:31PM +0100, Benjamin Block wrote: > On Tue, Feb 09, 2021 at 10:19:45PM +0000, Martin Wilck wrote: > > On Mon, 2021-02-08 at 23:19 -0600, Benjamin Marzinski wrote: > > > There are cases where the wwid of a path changes due to LUN remapping > > > without triggering uevent for the changed path. Multipathd has no > > > method > > > for trying to catch these cases, and corruption has resulted because > > > of > > > it. > > > > > > In order to have a better chance at catching these cases, multipath > > > now > > > has a recheck_wwid_time option, which can either be set to "off" or a > > > number of seconds. If a path is failed for equal to or greater than > > > the > > > configured number of seconds, multipathd will recheck its wwid before > > > restoring it, when the path checker sees that it has come back up. > > > > Can't the WWID change also happen without the path going offline, or > > at least without being offline long enough that multipathd would > > notice? > > > > > If > > > multipathd notices that a path's wwid has changed it will remove and > > > re-add the path, just like the existing wwid checking code for change > > > events does. In cases where the no uevent occurs, both the udev > > > database entry and sysfs will have the old wwid, so the only way to > > > get > > > a current wwid is to ask the device directly. > > > > sysfs is wrong too, really? In that case I fear triggering an uevent > > won't fix the situation. You need to force the kernel to rescan the > > device, otherwise udev will fetch the WWID from sysfs again, which > > still has the wrong ID... or what am I missing here? > > > > > > Currently multipath only > > > has code to directly get the wwid for scsi devices, so this option > > > only > > > effects scsi devices. Also, since it's possible the the udev wwid > > > won't > > > match the wwid from get_vpd_sgio(), if multipathd doesn't initially > > > see > > > the two values matching for a device, it will disable this option for > > > that device. > > > > > > If recheck_wwid_time is not turned off, multipathd will also > > > automatically recheck the wwid whenever an existing path gets a add > > > event, or is manually re-added with cli_add_path(). > > > > > > Co-developed-by: Chongyun Wu <wucy11@chinatelecom.cn> > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > > I am uncertain about this. > > > > We get one more configuration option that defaults to off and that only > > the truly inaugurated will understand and use. And even those will not > > know how to set the recheck time. Should it be 1s, 10, or 100? We > > already have too many of these options in multipath-tools. We shy away > > from giving users reasonable defaults, with the result that most people > > won't bother. > > > > I generally don't understand what the UP/DOWN state has to do with > > this. If the WWID can change without any events seen by either the > > kernel or user space, why would the path go down and up again? And even > > if so, why would it matter how long the device remained down? > > > > But foremost, do we really have to try to deal with configuration > > mistakes as blatant as this? What if a user sets the same WWID for > > different devices, or re-uses the same WWID on different storage > > servers? I already hesitated about the code I added myself for catching > > user errors in the WWIDs file, but this seems even more far-fetched. > > > > Please convince me. > > > > This said, I'd like to understand why there are no events in these > > cases. I'd have thought we'd at least get a UNIT ATTENTION (REPORTED > > LUNS DATA HAS CHANGED), which would have caused a uevent. If there was > > no UNIT ATTENTION, I'd blame the storage side. > > Yeah, just for reference, I saw this happening in practice when > something with the LU mapping changed on IBM storage - IIRC I saw it > with capacity changes. You end up in this code in the kernel: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_error.c?id=92bf22614b21a2706f4993b278017e437f7785b3#n416 > > And from there you ought to get an uevent for the sdev. > > The WWID in sysfs might still be wrong though AFAIK. The kernel seems to > ignore the UA after it delivered the uevent. > Sorry, I replied with the wrong mail address. -- Best Regards, Benjamin Block / Linux on IBM Z Kernel Development / IBM Systems IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy Vorsitz. AufsR.: Gregor Pillen / Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-09 22:19 ` Martin Wilck 2021-02-10 18:09 ` Benjamin Block @ 2021-02-11 4:48 ` Benjamin Marzinski 2021-02-11 12:14 ` Martin Wilck 2021-02-27 6:02 ` Benjamin Marzinski 2 siblings, 1 reply; 13+ messages in thread From: Benjamin Marzinski @ 2021-02-11 4:48 UTC (permalink / raw) To: Martin Wilck; +Cc: dm-devel, wucy11 On Tue, Feb 09, 2021 at 10:19:45PM +0000, Martin Wilck wrote: > On Mon, 2021-02-08 at 23:19 -0600, Benjamin Marzinski wrote: > > There are cases where the wwid of a path changes due to LUN remapping > > without triggering uevent for the changed path. Multipathd has no > > method > > for trying to catch these cases, and corruption has resulted because > > of > > it. > > > > In order to have a better chance at catching these cases, multipath > > now > > has a recheck_wwid_time option, which can either be set to "off" or a > > number of seconds. If a path is failed for equal to or greater than > > the > > configured number of seconds, multipathd will recheck its wwid before > > restoring it, when the path checker sees that it has come back up. > > Can't the WWID change also happen without the path going offline, or > at least without being offline long enough that multipathd would > notice? Yes. There is no way to guarantee that you won't hit this issue. That's why this is configurable and disableable. People get to choose how likely they are to shoot themselves in the foot. > > > If > > multipathd notices that a path's wwid has changed it will remove and > > re-add the path, just like the existing wwid checking code for change > > events does. In cases where the no uevent occurs, both the udev > > database entry and sysfs will have the old wwid, so the only way to > > get > > a current wwid is to ask the device directly. > > sysfs is wrong too, really? In that case I fear triggering an uevent > won't fix the situation. You need to force the kernel to rescan the > device, otherwise udev will fetch the WWID from sysfs again, which > still has the wrong ID... or what am I missing here? In the reproducer I posted using targetcli and FC devices, sysfs is wrong. That does make me a little leary about simply re-adding these devices as new. udev will run scsi_id to grab the new WWID and store that in the udev database, but if we ever fail back to looking at sysfs, we will still see the old data. In general, having devices on the system with bad sysfs data seems dangerous. > > > Currently multipath only > > has code to directly get the wwid for scsi devices, so this option > > only > > effects scsi devices. Also, since it's possible the the udev wwid > > won't > > match the wwid from get_vpd_sgio(), if multipathd doesn't initially > > see > > the two values matching for a device, it will disable this option for > > that device. > > > > If recheck_wwid_time is not turned off, multipathd will also > > automatically recheck the wwid whenever an existing path gets a add > > event, or is manually re-added with cli_add_path(). > > > > Co-developed-by: Chongyun Wu <wucy11@chinatelecom.cn> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > I am uncertain about this. > > We get one more configuration option that defaults to off and that only > the truly inaugurated will understand and use. And even those will not > know how to set the recheck time. Should it be 1s, 10, or 100? We > already have too many of these options in multipath-tools. We shy away > from giving users reasonable defaults, with the result that most people > won't bother. > > I generally don't understand what the UP/DOWN state has to do with > this. If the WWID can change without any events seen by either the > kernel or user space, why would the path go down and up again? And even > if so, why would it matter how long the device remained down? A new LUN can't get remapped to a still-being-used LUN id. The LUN that was previously mapped to that id must get unmapped first. That will cause IO and and checker commands to fail. So unless a LUN gets unmapped from a LUN id, and a new one remapped to that id quick enough for no IO and or checker commands to go to it, the path should go down. In the case that spurred this development, the path was down for hours before a new LUN was remapped to that ID. > But foremost, do we really have to try to deal with configuration > mistakes as blatant as this? What if a user sets the same WWID for > different devices, or re-uses the same WWID on different storage > servers? I already hesitated about the code I added myself for catching > user errors in the WWIDs file, but this seems even more far-fetched. > > Please convince me. Err.. An important customer corrupted their data and while they admit that they were at fault, it's hard for them to guarantee that something like this won't happen in the future, and they asked if multipath could do a better job of catching these sorts of mistakes. Obviously this is more convincing when it's coming from your customer. But the fact still stands that this has happened to multiple users even with our existing code to catch this. Since this isn't a problem that can always be fixed, the best we can do is try to catch it before something bad happens. If the path is remapped before it goes down, then corruption can happen before there is any possiblity of doing anything. That case is unsolvable. But if the path does go down when the LUN is unmapped, then there is a chance to catch this before damage is done. Now, obviously if you don't set this to 0, then it's possible that the path gets unmapped and goes down, but comes up before your timeout, and you don't catch it in time. Really, this is a dial that nobody that hasn't got bitten by this issue cares about, but everyone who has had this happen really wants to be there. > This said, I'd like to understand why there are no events in these > cases. I'd have thought we'd at least get a UNIT ATTENTION (REPORTED > LUNS DATA HAS CHANGED), which would have caused a uevent. If there was > no UNIT ATTENTION, I'd blame the storage side. Are you able to try the reproducer I targetcli FC reproducer I posted? > Maybe we need to monitor scsi_device uevents. > > Technical remarks below. > > > > --- > > libmultipath/config.c | 1 + > > libmultipath/config.h | 1 + > > libmultipath/configure.c | 4 +- > > libmultipath/configure.h | 2 + > > libmultipath/defaults.h | 1 + > > libmultipath/dict.c | 36 ++++++++++++ > > libmultipath/libmultipath.version | 6 ++ > > libmultipath/structs.h | 10 ++++ > > multipath/multipath.conf.5 | 18 ++++++ > > multipathd/cli_handlers.c | 9 +++ > > multipathd/main.c | 92 > > +++++++++++++++++++++++++++++++ > > multipathd/main.h | 2 + > > 12 files changed, 180 insertions(+), 2 deletions(-) > > . > > .\" ---------------------------------------------------------------- > > ------------ > > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c > > index 54635738..970d5e21 100644 > > --- a/multipathd/cli_handlers.c > > +++ b/multipathd/cli_handlers.c > > @@ -715,6 +715,15 @@ cli_add_path (void * v, char ** reply, int * > > len, void * data) > > pp = find_path_by_dev(vecs->pathvec, param); > > if (pp && pp->initialized != INIT_REMOVED) { > > condlog(2, "%s: path already in pathvec", param); > > + > > + if (pp->allow_wwid_recheck == ALLOW_WWID_RECHECK_ON > > && > > + check_path_wwid_change(pp)) { > > + condlog(0, "%s: wwid changed. Removing > > device", > > + pp->dev); > > + handle_path_wwid_change(pp, vecs); > > + return 1; > > + } > > + > > if (pp->mpp) > > return 0; > > } else if (pp) { > > diff --git a/multipathd/main.c b/multipathd/main.c > > index 19679848..17eef3a4 100644 > > --- a/multipathd/main.c > > +++ b/multipathd/main.c > > @@ -823,6 +823,59 @@ ev_remove_map (char * devname, char * alias, int > > minor, struct vectors * vecs) > > return flush_map(mpp, vecs, 0); > > } > > > > +void > > +handle_path_wwid_change(struct path *pp, struct vectors *vecs) > > +{ > > + const char *action = "add"; > > + if (!pp || !pp->udev) > > + return; > > + > > + sysfs_attr_set_value(pp->udev, "uevent", action, > > strlen(action)); > > + trigger_partitions_udev_change(pp->udev, action, > > strlen(action)); > > Nit: it looks a bit weird to use a const char * variable for "action" > and a constant for "uevent". It does mean that there's no chance of typo'ing it one of the four times it's used and not having it be caught, but I'm fine with changing it. > > + if (ev_remove_path(pp, vecs, 1) != 0 && pp->mpp) { > > + pp->dmstate = PSTATE_FAILED; > > + dm_fail_path(pp->mpp->alias, pp->dev_t); > > + } > > I suggest taking a ref on pp->udev, calling ev_remove_path(), and > triggering the uevent after that. > vecs locking will save us from handling the uevent before we remove the path, but your ordering does make things look more obviously correct. I'm fine with changing it. > > +} > > + > > +bool > > +check_path_wwid_change(struct path *pp) > > +{ > > + char wwid[WWID_SIZE]; > > + int len = 0; > > + char *c; > > + > > + if (!strlen(pp->wwid) || pp->bus != SYSFS_BUS_SCSI) > > + return false; > > Maybe you should look at uid_attribute here, to be consistent with > has_uid_fallback()? Possibly, be this code will never be run to see if wwid has changed without first having been run and verifying that the wwids match. If the wwids don't match the first time, then it's disabled. > > + > > + /* Get the real fresh device wwid by sgio. sysfs still has > > old > > + * data, so only get_vpd_sgio will work to get the new wwid > > */ > > + len = get_vpd_sgio(pp->fd, 0x83, 0, wwid, WWID_SIZE); > > + > > + if (len <= 0) { > > + condlog(2, "%s: failed to check wwid by sgio: len = > > %d", > > + pp->dev, len); > > + return false; > > + } > > + > > + /*Strip any trailing blanks */ > > + c = strchr(wwid, '\0'); > > Quite unusual, why not use strlen() or strnlen()? This was pulled directly from get_uid(). But I agree it could be cleaned up in both places > > > + c--; > > + while (c && c >= wwid && *c == ' ') { > > + *c = '\0'; > > + c--; > > + } > > Nit: You don't have to nullify every space. Just the first one. Again, this is just a copy of get_uid(). > > > + condlog(4, "%s: Got wwid %s by sgio", pp->dev, wwid); > > + > > + if (strncmp(wwid, pp->wwid, WWID_SIZE)) { > > + condlog(0, "%s: wwid '%s' doesn't match wwid '%s' > > from device", > > + pp->dev, pp->wwid, wwid); > > + return true; > > + } > > + > > + return false; > > +} > > + > > static int > > uev_add_path (struct uevent *uev, struct vectors * vecs, int > > need_do_map) > > { > > @@ -919,6 +972,21 @@ uev_add_path (struct uevent *uev, struct vectors > > * vecs, int need_do_map) > > uev->kernel); > > ret = 1; > > } > > + } else if (pp->allow_wwid_recheck == > > ALLOW_WWID_RECHECK_ON && > > + check_path_wwid_change(pp)) { > > + condlog(2, "%s wwid change detected when > > processing add uevent. removing path", pp->dev); > > + /* > > + * don't use handle_path_wwid_change here, > > + * since that would just trigger another add > > + * uevent > > + */ > > + ret = ev_remove_path(pp, vecs, true); > > + if (ret == 0) > > + pp = NULL; > > + else if (pp->mpp) { > > + pp->dmstate = PSTATE_FAILED; > > + dm_fail_path(pp->mpp->alias, pp- > > >dev_t); > > + } > > What's the purpose of this code? Are you handling your own artificial > "add" event here, which you triggered before in > handle_path_wwid_change()? Or are there real cases where the kernel > would send an "add" event without sending a "remove" event first? > This shouldn't be for handling our own add event. Either the ev_remove_path() in handle_path_wwid_changed() succeeded, and there is no path in pathvec, or it failed, and pp->initialized should be set to INIT_REMOVED. Either way, we miss this code path. This is simply if we get an "add" event without the "remove" event, which is a real thing that has happened when LUNs get remapped. > > } > > } > > if (pp) > > @@ -2049,6 +2117,7 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > unsigned int checkint, max_checkint; > > struct config *conf; > > int marginal_pathgroups, marginal_changed = 0; > > + int recheck_wwid_time; > > int ret; > > > > if (((pp->initialized == INIT_OK || > > @@ -2066,6 +2135,7 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > checkint = conf->checkint; > > max_checkint = conf->max_checkint; > > marginal_pathgroups = conf->marginal_pathgroups; > > + recheck_wwid_time = conf->recheck_wwid_time; > > put_multipath_config(conf); > > > > if (pp->checkint == CHECKINT_UNDEF) { > > @@ -2197,6 +2267,26 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > return 0; > > set_no_path_retry(pp->mpp); > > > > + if (recheck_wwid_time != RECHECK_WWID_OFF && > > + (newstate == PATH_UP || newstate == PATH_GHOST)) { > > + if (pp->allow_wwid_recheck == > > ALLOW_WWID_RECHECK_UNSET) { > > + if (check_path_wwid_change(pp)) > > + pp->allow_wwid_recheck = > > ALLOW_WWID_RECHECK_OFF; > > + else > > + pp->allow_wwid_recheck = > > ALLOW_WWID_RECHECK_ON; > > This is confusing. I pulled my hair over this code before I read your > man page hunk: "When the path is originally added, if the path's > configured wwid doesn't match the wwid retrieved directly > from the scsi device, rechecks will be disabled for the device." > > So, I gather this is an alternative to the has_uid_fallback() logic > mentioned above. It deserves at least a comment here. Please consider > if using the same logic as has_uid_falback() isn't just as good as > this. I'm fine with using the same logic as has_uid_fallback() to determine if if we can use get_vpd_sgio(). -Ben > Btw, as we're already pretty much on corner-case ground here, if the > path fails quickly after being detected, a WWID change could have > occured already when it comes UP first, and this code is run for the > first time. > > > > + } else if (((pp->state != PATH_UP && pp->state != > > PATH_GHOST) || > > + pp->dmstate == PSTATE_FAILED) && > > + pp->failed_time >= recheck_wwid_time && > > + pp->allow_wwid_recheck == > > ALLOW_WWID_RECHECK_ON && > > + check_path_wwid_change(pp)) { > > + condlog(0, "%s: path wwid change detected. > > Removing", > > + pp->dev); > > + handle_path_wwid_change(pp, vecs); > > + return 0; > > + } > > + pp->failed_time = 0; > > + } > > + > > if ((newstate == PATH_UP || newstate == PATH_GHOST) && > > (san_path_check_enabled(pp->mpp) || > > marginal_path_check_enabled(pp->mpp))) { > > @@ -2330,6 +2420,8 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > if (newstate == PATH_DOWN) { > > int log_checker_err; > > > > + if (recheck_wwid_time != RECHECK_WWID_OFF) > > + pp->failed_time += pp->checkint; > > conf = get_multipath_config(); > > log_checker_err = conf->log_checker_err; > > put_multipath_config(conf); > > diff --git a/multipathd/main.h b/multipathd/main.h > > index 5abbe97b..ddd953f9 100644 > > --- a/multipathd/main.h > > +++ b/multipathd/main.h > > @@ -50,4 +50,6 @@ int update_multipath (struct vectors *vecs, char > > *mapname, int reset); > > int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs, > > int refresh); > > > > +void handle_path_wwid_change(struct path *pp, struct vectors *vecs); > > +bool check_path_wwid_change(struct path *pp); > > #endif /* MAIN_H */ > > -- > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 > SUSE Software Solutions Germany GmbH > HRB 36809, AG Nürnberg GF: Felix Imendörffer > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-11 4:48 ` Benjamin Marzinski @ 2021-02-11 12:14 ` Martin Wilck 2021-02-18 3:22 ` Chongyun Wu 0 siblings, 1 reply; 13+ messages in thread From: Martin Wilck @ 2021-02-11 12:14 UTC (permalink / raw) To: bmarzins; +Cc: dm-devel, wucy11 On Wed, 2021-02-10 at 22:48 -0600, Benjamin Marzinski wrote: > On Tue, Feb 09, 2021 at 10:19:45PM +0000, Martin Wilck wrote: > > On Mon, 2021-02-08 at 23:19 -0600, Benjamin Marzinski wrote: > > > There are cases where the wwid of a path changes due to LUN > > > remapping > > > without triggering uevent for the changed path. Multipathd has no > > > method > > > for trying to catch these cases, and corruption has resulted > > > because > > > of > > > it. > > > > > > In order to have a better chance at catching these cases, > > > multipath > > > now > > > has a recheck_wwid_time option, which can either be set to "off" > > > or a > > > number of seconds. If a path is failed for equal to or greater > > > than > > > the > > > configured number of seconds, multipathd will recheck its wwid > > > before > > > restoring it, when the path checker sees that it has come back > > > up. > > > > Can't the WWID change also happen without the path going offline, > > or > > at least without being offline long enough that multipathd would > > notice? > > Yes. There is no way to guarantee that you won't hit this issue. > That's > why this is configurable and disableable. People get to choose how > likely they are to shoot themselves in the foot. We have seen quite a few cases lately where TUR checker was "wrong" about path state. TUR would succeed, but the regular IO on the path would either fail or, worse, hang. So by adding an SG_IO right in the middle of check_path(), with vecs lock held, we increase the risk of making multipathd hang (it can happen elsewhere, too, like in get_prio(); I'm still pondering how to avoid it in general). Thus, it's definitely reasonable to let users switch this on and off. (it should perhaps be a hwtable option. Are these WWID configuration mistakes more likely to happen on specific storage arrays than on others?). But I fail to see the reason of making the likelihood configurable. Every sane user would strive for a minimum likelihood for this kind of failure. As you pointed out, we can't guarantee a zero likelihood, but *if* we do this, and *if* we do it when paths come back alive as you implemented it, then it makes sense to do it always as quickly as possible. IOW: I think it should be a boolean option, and if "on", it should test after every down time. > > > > > > If > > > multipathd notices that a path's wwid has changed it will remove > > > and > > > re-add the path, just like the existing wwid checking code for > > > change > > > events does. In cases where the no uevent occurs, both the udev > > > database entry and sysfs will have the old wwid, so the only way > > > to > > > get > > > a current wwid is to ask the device directly. > > > > sysfs is wrong too, really? In that case I fear triggering an > > uevent > > won't fix the situation. You need to force the kernel to rescan the > > device, otherwise udev will fetch the WWID from sysfs again, which > > still has the wrong ID... or what am I missing here? > > In the reproducer I posted using targetcli and FC devices, sysfs is > wrong. That does make me a little leary about simply re-adding these > devices as new. udev will run scsi_id to grab the new WWID and store > that in the udev database, but if we ever fail back to looking at > sysfs, > we will still see the old data. In general, having devices on the > system with bad sysfs data seems dangerous. That's right, we shouldn't do this. Either we force a SCSI-level rescan and verify the WWID is correct in sysfs therafter, or we mark this path as unusable/bad and spit out BIG FAT ERROR MESSAGES. > > > > > Currently multipath only > > > has code to directly get the wwid for scsi devices, so this > > > option > > > only > > > effects scsi devices. Also, since it's possible the the udev wwid > > > won't > > > match the wwid from get_vpd_sgio(), if multipathd doesn't > > > initially > > > see > > > the two values matching for a device, it will disable this option > > > for > > > that device. > > > > > > If recheck_wwid_time is not turned off, multipathd will also > > > automatically recheck the wwid whenever an existing path gets a > > > add > > > event, or is manually re-added with cli_add_path(). > > > > > > Co-developed-by: Chongyun Wu <wucy11@chinatelecom.cn> > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > > I am uncertain about this. > > > > We get one more configuration option that defaults to off and that > > only > > the truly inaugurated will understand and use. And even those will > > not > > know how to set the recheck time. Should it be 1s, 10, or 100? We > > already have too many of these options in multipath-tools. We shy > > away > > from giving users reasonable defaults, with the result that most > > people > > won't bother. > > > > I generally don't understand what the UP/DOWN state has to do with > > this. If the WWID can change without any events seen by either the > > kernel or user space, why would the path go down and up again? And > > even > > if so, why would it matter how long the device remained down? > > A new LUN can't get remapped to a still-being-used LUN id. The LUN > that > was previously mapped to that id must get unmapped first. That will > cause IO and and checker commands to fail. So unless a LUN gets > unmapped > from a LUN id, and a new one remapped to that id quick enough for no > IO > and or checker commands to go to it, the path should go down. In the > case that spurred this development, the path was down for hours > before a > new LUN was remapped to that ID. OK. My thinking was that storage configuration changes are made by scripts or software more often than not, and in this case down times may be quite short. But if they say they need it - fair enough. > > > But foremost, do we really have to try to deal with configuration > > mistakes as blatant as this? What if a user sets the same WWID for > > different devices, or re-uses the same WWID on different storage > > servers? I already hesitated about the code I added myself for > > catching > > user errors in the WWIDs file, but this seems even more far- > > fetched. > > > > Please convince me. > > Err.. An important customer corrupted their data and while they admit > that they were at fault, it's hard for them to guarantee that > something > like this won't happen in the future, and they asked if multipath > could > do a better job of catching these sorts of mistakes. Obviously this > is > more convincing when it's coming from your customer. But the fact > still > stands that this has happened to multiple users even with our > existing > code to catch this. I wasn't aware of multiple affected users. I saw Chongyun's post and it looked to me as if this had happend once, likely in his organization. It wasn't even clear to me whether production data were affected. I still believe that this is primarily a problem on the storage vendor side. They should make it harder for users to shoot themselves in the foot (low-level LIO doesn't count - of course you can do all kinds of ugly stuff with it). > Since this isn't a problem that can always be fixed, the best we can > do > is try to catch it before something bad happens. If the path is > remapped before it goes down, then corruption can happen before there > is any possiblity of doing anything. That case is unsolvable. But if > the path does go down when the LUN is unmapped, then there is a > chance > to catch this before damage is done. > > Now, obviously if you don't set this to 0, then it's possible that > the > path gets unmapped and goes down, but comes up before your timeout, > and > you don't catch it in time. Really, this is a dial that nobody that > hasn't got bitten by this issue cares about, but everyone who has had > this happen really wants to be there. OK, then. Please consider making it boolean though, as argued above. If the time must _really_ be configurable, maybe those affected parties can come up with reasonable suggestions for a default? > > > This said, I'd like to understand why there are no events in these > > cases. I'd have thought we'd at least get a UNIT ATTENTION > > (REPORTED > > LUNS DATA HAS CHANGED), which would have caused a uevent. If there > > was > > no UNIT ATTENTION, I'd blame the storage side. > > Are you able to try the reproducer I targetcli FC reproducer I > posted? I wanted to, but I had no time to set this up yet. > > > Maybe we need to monitor scsi_device uevents. > > > > Technical remarks below. > > > > > > > --- > > > libmultipath/config.c | 1 + > > > libmultipath/config.h | 1 + > > > libmultipath/configure.c | 4 +- > > > libmultipath/configure.h | 2 + > > > libmultipath/defaults.h | 1 + > > > libmultipath/dict.c | 36 ++++++++++++ > > > libmultipath/libmultipath.version | 6 ++ > > > libmultipath/structs.h | 10 ++++ > > > multipath/multipath.conf.5 | 18 ++++++ > > > multipathd/cli_handlers.c | 9 +++ > > > multipathd/main.c | 92 > > > +++++++++++++++++++++++++++++++ > > > multipathd/main.h | 2 + > > > 12 files changed, 180 insertions(+), 2 deletions(-) > > > . > > > .\" ------------------------------------------------------------ > > > ---- > > > ------------ > > > diff --git a/multipathd/cli_handlers.c > > > b/multipathd/cli_handlers.c > > > index 54635738..970d5e21 100644 > > > --- a/multipathd/cli_handlers.c > > > +++ b/multipathd/cli_handlers.c > > > @@ -715,6 +715,15 @@ cli_add_path (void * v, char ** reply, int * > > > len, void * data) > > > pp = find_path_by_dev(vecs->pathvec, param); > > > if (pp && pp->initialized != INIT_REMOVED) { > > > condlog(2, "%s: path already in pathvec", param); > > > + > > > + if (pp->allow_wwid_recheck == > > > ALLOW_WWID_RECHECK_ON > > > && > > > + check_path_wwid_change(pp)) { > > > + condlog(0, "%s: wwid changed. Removing > > > device", > > > + pp->dev); > > > + handle_path_wwid_change(pp, vecs); > > > + return 1; > > > + } > > > + > > > if (pp->mpp) > > > return 0; > > > } else if (pp) { > > > diff --git a/multipathd/main.c b/multipathd/main.c > > > index 19679848..17eef3a4 100644 > > > --- a/multipathd/main.c > > > +++ b/multipathd/main.c > > > @@ -823,6 +823,59 @@ ev_remove_map (char * devname, char * alias, > > > int > > > minor, struct vectors * vecs) > > > return flush_map(mpp, vecs, 0); > > > } > > > > > > +void > > > +handle_path_wwid_change(struct path *pp, struct vectors *vecs) > > > +{ > > > + const char *action = "add"; > > > + if (!pp || !pp->udev) > > > + return; > > > + > > > + sysfs_attr_set_value(pp->udev, "uevent", action, > > > strlen(action)); > > > + trigger_partitions_udev_change(pp->udev, action, > > > strlen(action)); > > > > Nit: it looks a bit weird to use a const char * variable for > > "action" > > and a constant for "uevent". > > It does mean that there's no chance of typo'ing it one of the four > times > it's used and not having it be caught, but I'm fine with changing it. > > > > + if (ev_remove_path(pp, vecs, 1) != 0 && pp->mpp) { > > > + pp->dmstate = PSTATE_FAILED; > > > + dm_fail_path(pp->mpp->alias, pp->dev_t); > > > + } > > > > I suggest taking a ref on pp->udev, calling ev_remove_path(), and > > triggering the uevent after that. > > > > vecs locking will save us from handling the uevent before we remove > the > path, but your ordering does make things look more obviously correct. > I'm fine with changing it. > > > > +} > > > + > > > +bool > > > +check_path_wwid_change(struct path *pp) > > > +{ > > > + char wwid[WWID_SIZE]; > > > + int len = 0; > > > + char *c; > > > + > > > + if (!strlen(pp->wwid) || pp->bus != SYSFS_BUS_SCSI) > > > + return false; > > > > Maybe you should look at uid_attribute here, to be consistent with > > has_uid_fallback()? > > Possibly, be this code will never be run to see if wwid has changed > without first having been run and verifying that the wwids match. If > the wwids don't match the first time, then it's disabled. > > > > + > > > + /* Get the real fresh device wwid by sgio. sysfs still > > > has > > > old > > > + * data, so only get_vpd_sgio will work to get the new > > > wwid > > > */ > > > + len = get_vpd_sgio(pp->fd, 0x83, 0, wwid, WWID_SIZE); > > > + > > > + if (len <= 0) { > > > + condlog(2, "%s: failed to check wwid by sgio: len > > > = > > > %d", > > > + pp->dev, len); > > > + return false; > > > + } > > > + > > > + /*Strip any trailing blanks */ > > > + c = strchr(wwid, '\0'); > > > > Quite unusual, why not use strlen() or strnlen()? > > This was pulled directly from get_uid(). But I agree it could be > cleaned > up in both places Strange, it never caught my eye. Maybe we should do a start-from-zero review of our code :-/ > > > > > > + c--; > > > + while (c && c >= wwid && *c == ' ') { > > > + *c = '\0'; > > > + c--; > > > + } > > > > Nit: You don't have to nullify every space. Just the first one. > Again, this is just a copy of get_uid(). > > > > > > + condlog(4, "%s: Got wwid %s by sgio", pp->dev, wwid); > > > + > > > + if (strncmp(wwid, pp->wwid, WWID_SIZE)) { > > > + condlog(0, "%s: wwid '%s' doesn't match wwid '%s' > > > from device", > > > + pp->dev, pp->wwid, wwid); > > > + return true; > > > + } > > > + > > > + return false; > > > +} > > > + > > > static int > > > uev_add_path (struct uevent *uev, struct vectors * vecs, int > > > need_do_map) > > > { > > > @@ -919,6 +972,21 @@ uev_add_path (struct uevent *uev, struct > > > vectors > > > * vecs, int need_do_map) > > > uev->kernel); > > > ret = 1; > > > } > > > + } else if (pp->allow_wwid_recheck == > > > ALLOW_WWID_RECHECK_ON && > > > + check_path_wwid_change(pp)) { > > > + condlog(2, "%s wwid change detected when > > > processing add uevent. removing path", pp->dev); > > > + /* > > > + * don't use handle_path_wwid_change > > > here, > > > + * since that would just trigger another > > > add > > > + * uevent > > > + */ > > > + ret = ev_remove_path(pp, vecs, true); > > > + if (ret == 0) > > > + pp = NULL; > > > + else if (pp->mpp) { > > > + pp->dmstate = PSTATE_FAILED; > > > + dm_fail_path(pp->mpp->alias, pp- > > > > dev_t); > > > + } > > > > What's the purpose of this code? Are you handling your own > > artificial > > "add" event here, which you triggered before in > > handle_path_wwid_change()? Or are there real cases where the kernel > > would send an "add" event without sending a "remove" event first? > > > > This shouldn't be for handling our own add event. Either the > ev_remove_path() in handle_path_wwid_changed() succeeded, and there > is > no path in pathvec, or it failed, and pp->initialized should be set > to > INIT_REMOVED. Either way, we miss this code path. This is simply if > we > get an "add" event without the "remove" event, which is a real thing > that has happened when LUNs get remapped. That sounds like a kernel bug, or something else really awkward and bad which we should try to understand and fix. Thanks, Martin -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-11 12:14 ` Martin Wilck @ 2021-02-18 3:22 ` Chongyun Wu 2021-02-19 10:46 ` Martin Wilck 0 siblings, 1 reply; 13+ messages in thread From: Chongyun Wu @ 2021-02-18 3:22 UTC (permalink / raw) To: Martin Wilck, bmarzins; +Cc: dm-devel On 2021/2/11 20:14, Martin Wilck wrote: >>> But foremost, do we really have to try to deal with configuration >>> mistakes as blatant as this? What if a user sets the same WWID for >>> different devices, or re-uses the same WWID on different storage >>> servers? I already hesitated about the code I added myself for >>> catching >>> user errors in the WWIDs file, but this seems even more far- >>> fetched. >>> >>> Please convince me. >> >> Err.. An important customer corrupted their data and while they admit >> that they were at fault, it's hard for them to guarantee that >> something >> like this won't happen in the future, and they asked if multipath >> could >> do a better job of catching these sorts of mistakes. Obviously this >> is >> more convincing when it's coming from your customer. But the fact >> still >> stands that this has happened to multiple users even with our >> existing >> code to catch this. > > I wasn't aware of multiple affected users. I saw Chongyun's post and it > looked to me as if this had happend once, likely in his organization. > It wasn't even clear to me whether production data were affected. > Had customers have made incorrect storage back-end operations during the expansion process, which has caused the destruction of customer data, which cannot be repaired using any tools. And I reproduce it in our lab too. Customers definitely hope that multipath-tools can probide protection in this case, becuase if it is important custmer data storage in the LUN, the loss will be great. -- Best Regard, Chongyun Wu -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-18 3:22 ` Chongyun Wu @ 2021-02-19 10:46 ` Martin Wilck 0 siblings, 0 replies; 13+ messages in thread From: Martin Wilck @ 2021-02-19 10:46 UTC (permalink / raw) To: wucy11, bmarzins; +Cc: dm-devel On Thu, 2021-02-18 at 11:22 +0800, Chongyun Wu wrote: > > > > I wasn't aware of multiple affected users. I saw Chongyun's post > > and it > > looked to me as if this had happend once, likely in his > > organization. > > It wasn't even clear to me whether production data were affected. > > > Had customers have made incorrect storage back-end operations during > the > expansion process, which has caused the destruction of customer data, > which > cannot be repaired using any tools. And I reproduce it in our lab > too. > Customers definitely hope that multipath-tools can probide protection > in > this case, becuase if it is important custmer data storage in the > LUN, the > loss will be great. I understand that. My concern was that it's still just quite a weak protection, because it depends on the check time interval being short enough to catch the change. So in a way, this pretends to offer safety that doesn't truly exist. If the user assigns the wrong WWID 30s after taking the LUN offline, all fine; if she is quicker (or has the process automated / scripted), data corruption still occurs. And of course, there are general limits to which extent software can avoid user mistakes. However, I trust your and Ben's judegement that by checking this in a suitably short time span, the issues your customers have had would have been avoided, and I'm not going to oppose the patch on these grounds. Martin -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid 2021-02-09 22:19 ` Martin Wilck 2021-02-10 18:09 ` Benjamin Block 2021-02-11 4:48 ` Benjamin Marzinski @ 2021-02-27 6:02 ` Benjamin Marzinski 2 siblings, 0 replies; 13+ messages in thread From: Benjamin Marzinski @ 2021-02-27 6:02 UTC (permalink / raw) To: Martin Wilck; +Cc: dm-devel, wucy11 On Tue, Feb 09, 2021 at 10:19:45PM +0000, Martin Wilck wrote: > On Mon, 2021-02-08 at 23:19 -0600, Benjamin Marzinski wrote: > > If > > multipathd notices that a path's wwid has changed it will remove and > > re-add the path, just like the existing wwid checking code for change > > events does. In cases where the no uevent occurs, both the udev > > database entry and sysfs will have the old wwid, so the only way to > > get > > a current wwid is to ask the device directly. > > sysfs is wrong too, really? In that case I fear triggering an uevent > won't fix the situation. You need to force the kernel to rescan the > device, otherwise udev will fetch the WWID from sysfs again, which > still has the wrong ID... or what am I missing here? I just figured out the confusion here. Redhat has not always included 55-scsi-sg3_id.rules in sg3_utils. It does now, but installs it as 61-scsi-sg3_id.rules instead (This is why we install multipath.rules as 62-multipath.rules). The result of this is that ID_SERIAL is set by "scsi_id --export --whitelisted -d $devnode" from 60-persistent-storage.rules. This means that when a uevent comes in, ID_SERIAL is repopulated by ioctling the device, unlike the case where 55-scsi-sg3_id.rules sets ID_SERIAL, which gets the information from sysfs if possible. Clearly, for distributions that set ID_SERIAL from 55-scsi-sg3_id.rules, multipathd needs to take more drastic measures if the sysfs vpd_page83 file is wrong. Looking at this made me notice another issue. scsi_id and the rules in 55-scsi-sg3_id.rules don't agree on the priority of the different id types for choosing the wwid from the various ids in vpd page 83. And mulitpath's parse_vpd_pg83() doesn't agree with either of them. Here are the different priority orderings for the ID types sg_inq + scsi_id 55-scsi-sg3_id.rules parse_vpd_pg83 ------------------------------------------------------- 36 36 36 35 35 35 3[^56] 32 32 2 2 8 8 2 1 1 1 3[^652] 3[^652] 0 0 0 Since Redhat is changing the default ordering of the 55-scsi-sg3_id.rules, it seems reasonable that we make parse_vpd_pg83() match the 55-scsi-sg3_id.rules ordering, and I'll add a redhat specific patch to match it to the scsi_id ordering. However, the fact that differences which udev rules are installed or their ordering can changed the WWID does lend some weight to the code that I originally had for recheck_wwid, where we would only enable it after verifying that at one point, the parse_vpd_pg83() wwid agreed with the uid_attribute wwid. On the other hand, assuming that distributions make sure that parse_vpd_pg83() matches their udev setup, then not requiring them to be equal at some point allows recheck_wwid to catch problems that happened before multipath started using the device, for whatever that's worth. -Ben -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-02-27 6:03 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-09 5:19 [dm-devel] [PATCH 0/2] Handle remapped LUNs better Benjamin Marzinski 2021-02-09 5:19 ` [dm-devel] [PATCH 1/2] libmultipath: fix use-after-free in uev_add_path Benjamin Marzinski 2021-02-09 20:57 ` Martin Wilck 2021-02-09 5:19 ` [dm-devel] [PATCH 2/2] multipathd: add recheck_wwid_time option to verify the path wwid Benjamin Marzinski 2021-02-09 22:19 ` Martin Wilck 2021-02-10 18:09 ` Benjamin Block 2021-02-10 19:57 ` Martin Wilck 2021-02-11 11:25 ` Benjamin Block 2021-02-11 4:48 ` Benjamin Marzinski 2021-02-11 12:14 ` Martin Wilck 2021-02-18 3:22 ` Chongyun Wu 2021-02-19 10:46 ` Martin Wilck 2021-02-27 6:02 ` Benjamin Marzinski
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).