* [PATCH 00/10] misc. multipath patches.
@ 2016-10-29 2:55 Benjamin Marzinski
2016-10-29 2:55 ` [PATCH 01/10] libmultipath: add skip_kpartx option Benjamin Marzinski
` (9 more replies)
0 siblings, 10 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
Here are a number of multipath patches. The first two are resends of the
skip_kpartx patches that didn't get merged from my last patchset. I didn't see
any complaints about them, so I just rebased them. If there are any questions
or objections, please let me know.
The third and ninth patches both deal with paths that change wwids while in
use. Ususally this happens because of LUN remapping, which is not multipath's
fault. However, not dealing with it can cause multipathd to crash. It's also
nice to give users the chance to disable the device before it accidentally
writes to the wrong LUN.
The INIT_REQUESTED_UDEV code is currently broken, but my fix does mean
that multipathd now grabs the vecs lock on all path change events. It did
this for years, so I don't know it that is controversial or not. If it is
I can probably work around it. The disable_changed_wwids also needs to grab
the vecs lock on change events, but only if the user enables it (it's off
by default).
Benjamin Marzinski (10):
libmultipath: add skip_kpartx option
kpartx.rules: respect skip_kpartx flag
do not allow in-use path to change wwid
multipathd: add "map failures" format wildcard
mpath: don't wait for udev if all paths are down
multipath: set cookie before using it.
recover from errors in multipathd startup
fix INIT_REQUESTED_UDEV code
add disable_changed_wwids option
set retrigger_tries to 0 for multipath
kpartx/kpartx.rules | 2 +
libmultipath/config.c | 3 +
libmultipath/config.h | 4 ++
libmultipath/configure.c | 13 +++--
libmultipath/defaults.h | 2 +
libmultipath/devmapper.c | 47 +++++++++++-----
libmultipath/devmapper.h | 8 ++-
libmultipath/dict.c | 17 ++++++
libmultipath/discovery.c | 15 +++--
libmultipath/discovery.h | 1 +
libmultipath/dmparser.c | 8 +++
libmultipath/print.c | 7 +++
libmultipath/propsel.c | 18 ++++++
libmultipath/propsel.h | 1 +
libmultipath/structs.h | 9 +++
libmultipath/structs_vec.c | 28 ++++++----
multipath/main.c | 1 +
multipath/multipath.conf.5 | 17 ++++++
multipathd/cli_handlers.c | 15 ++++-
multipathd/main.c | 136 +++++++++++++++++++++++++++++----------------
20 files changed, 261 insertions(+), 91 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 01/10] libmultipath: add skip_kpartx option
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:42 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 02/10] kpartx.rules: respect skip_kpartx flag Benjamin Marzinski
` (8 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
This option gives multipath the ability to stop kpartx from running. The
previous idea, the "no_partitions" feature, was not accepted in the
upstream kernel. This method uses one of the dm cookie subsystem flags
DM_SUBSYSTEM_UDEV_FLAG1, which can be checked by udev to skip running
kpartx when processing the event. This patch does most of the work
necessary to make this work. It doesn't change kpartx.rules, however.
Also, if dm_suspend_and_flush_map fails, multipath doesn't know how the
device is configured. so, it simply checks if the device has any
partitions before attempting the remove, and if not, sets the
DM_SUBSYSTEM_UDEV_FLAG1 on the resume after failure, so that no
partitions will be generated.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/config.c | 2 ++
libmultipath/config.h | 3 +++
libmultipath/configure.c | 5 +++--
libmultipath/defaults.h | 1 +
libmultipath/devmapper.c | 45 +++++++++++++++++++++++++++++++--------------
libmultipath/devmapper.h | 8 +++++++-
libmultipath/dict.c | 13 +++++++++++++
libmultipath/propsel.c | 18 ++++++++++++++++++
libmultipath/propsel.h | 1 +
libmultipath/structs.h | 7 +++++++
multipath/multipath.conf.5 | 17 +++++++++++++++++
multipathd/cli_handlers.c | 4 +++-
12 files changed, 106 insertions(+), 18 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index ed51afb..bdcad80 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -347,6 +347,7 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
merge_num(deferred_remove);
merge_num(delay_watch_checks);
merge_num(delay_wait_checks);
+ merge_num(skip_kpartx);
/*
* Make sure features is consistent with
@@ -616,6 +617,7 @@ load_config (char * file)
conf->retrigger_delay = DEFAULT_RETRIGGER_DELAY;
conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT;
conf->deferred_remove = DEFAULT_DEFERRED_REMOVE;
+ conf->skip_kpartx = DEFAULT_SKIP_KPARTX;
/*
* preload default hwtable
diff --git a/libmultipath/config.h b/libmultipath/config.h
index a41207a..d59a993 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -65,6 +65,7 @@ struct hwentry {
int deferred_remove;
int delay_watch_checks;
int delay_wait_checks;
+ int skip_kpartx;
char * bl_product;
};
@@ -91,6 +92,7 @@ struct mpentry {
int deferred_remove;
int delay_watch_checks;
int delay_wait_checks;
+ int skip_kpartx;
uid_t uid;
gid_t gid;
mode_t mode;
@@ -141,6 +143,7 @@ struct config {
int ignore_new_devs;
int delayed_reconfig;
int uev_wait_timeout;
+ int skip_kpartx;
unsigned int version[3];
char * multipath_dir;
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 707e6be..48f100b 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -295,6 +295,7 @@ setup_map (struct multipath * mpp, char * params, int params_size)
select_deferred_remove(conf, mpp);
select_delay_watch_checks(conf, mpp);
select_delay_wait_checks(conf, mpp);
+ select_skip_kpartx(conf, mpp);
sysfs_set_scsi_tmo(mpp, conf->checkint);
put_multipath_config(conf);
@@ -641,14 +642,14 @@ domap (struct multipath * mpp, char * params, int is_daemon)
case ACT_RENAME:
conf = get_multipath_config();
r = dm_rename(mpp->alias_old, mpp->alias,
- conf->partition_delim);
+ conf->partition_delim, mpp->skip_kpartx);
put_multipath_config(conf);
break;
case ACT_FORCERENAME:
conf = get_multipath_config();
r = dm_rename(mpp->alias_old, mpp->alias,
- conf->partition_delim);
+ conf->partition_delim, mpp->skip_kpartx);
put_multipath_config(conf);
if (r)
r = dm_addmap_reload(mpp, params, 0);
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 9af9a9a..a1fee9b 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -35,6 +35,7 @@
#define DEFAULT_USER_FRIENDLY_NAMES USER_FRIENDLY_NAMES_OFF
#define DEFAULT_FORCE_SYNC 0
#define DEFAULT_PARTITION_DELIM NULL
+#define DEFAULT_SKIP_KPARTX SKIP_KPARTX_OFF
#define DEFAULT_CHECKINT 5
#define MAX_CHECKINT(a) (a << 2)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 89aa5da..ee61ff0 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -213,8 +213,9 @@ dm_prereq (void)
static int
dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t udev_flags, int deferred_remove) {
int r = 0;
- int udev_wait_flag = (need_sync && (task == DM_DEVICE_RESUME ||
- task == DM_DEVICE_REMOVE));
+ int udev_wait_flag = ((need_sync || udev_flags) &&
+ (task == DM_DEVICE_RESUME ||
+ task == DM_DEVICE_REMOVE));
uint32_t cookie = 0;
struct dm_task *dmt;
@@ -266,11 +267,12 @@ dm_device_remove (const char *name, int needsync, int deferred_remove) {
static int
dm_addmap (int task, const char *target, struct multipath *mpp,
- char * params, int ro) {
+ char * params, int ro, int skip_kpartx) {
int r = 0;
struct dm_task *dmt;
char *prefixed_uuid = NULL;
uint32_t cookie = 0;
+ uint16_t udev_flags = DM_UDEV_DISABLE_LIBRARY_FALLBACK | ((skip_kpartx == SKIP_KPARTX_ON)? MPATH_UDEV_NO_KPARTX_FLAG : 0);
if (!(dmt = dm_task_create (task)))
return 0;
@@ -319,8 +321,7 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
dm_task_no_open_count(dmt);
if (task == DM_DEVICE_CREATE &&
- !dm_task_set_cookie(dmt, &cookie,
- DM_UDEV_DISABLE_LIBRARY_FALLBACK))
+ !dm_task_set_cookie(dmt, &cookie, udev_flags))
goto freeout;
r = dm_task_run (dmt);
@@ -344,7 +345,8 @@ dm_addmap_create (struct multipath *mpp, char * params) {
for (ro = 0; ro <= 1; ro++) {
int err;
- if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro))
+ if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro,
+ mpp->skip_kpartx))
return 1;
/*
* DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
@@ -371,7 +373,9 @@ extern int
dm_addmap_reload (struct multipath *mpp, char *params, int flush)
{
int r;
- uint16_t udev_flags = flush ? 0 : MPATH_UDEV_RELOAD_FLAG;
+ uint16_t udev_flags = (flush ? 0 : MPATH_UDEV_RELOAD_FLAG) |
+ ((mpp->skip_kpartx == SKIP_KPARTX_ON)?
+ MPATH_UDEV_NO_KPARTX_FLAG : 0);
/*
* DM_DEVICE_RELOAD cannot wait on a cookie, as
@@ -379,12 +383,13 @@ dm_addmap_reload (struct multipath *mpp, char *params, int flush)
* DM_DEVICE_RESUME. So call DM_DEVICE_RESUME
* after each successful call to DM_DEVICE_RELOAD.
*/
- r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RW);
+ r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RW,
+ SKIP_KPARTX_OFF);
if (!r) {
if (errno != EROFS)
return 0;
r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp,
- params, ADDMAP_RO);
+ params, ADDMAP_RO, SKIP_KPARTX_OFF);
}
if (r)
r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias, flush,
@@ -761,6 +766,12 @@ out:
}
static int
+has_partmap(const char *name, void *data)
+{
+ return 1;
+}
+
+static int
partmap_in_use(const char *name, void *data)
{
int part_count, *ret_count = (int *)data;
@@ -839,10 +850,16 @@ dm_suspend_and_flush_map (const char * mapname)
int s = 0, queue_if_no_path = 0;
unsigned long long mapsize;
char params[PARAMS_SIZE] = {0};
+ int udev_flags = 0;
if (!dm_is_mpath(mapname))
return 0; /* nothing to do */
+ /* if the device currently has no partitions, do not
+ run kpartx on it if you fail to delete it */
+ if (do_foreach_partmaps(mapname, has_partmap, NULL) == 0)
+ udev_flags |= MPATH_UDEV_NO_KPARTX_FLAG;
+
if (!dm_get_map(mapname, &mapsize, params)) {
if (strstr(params, "queue_if_no_path"))
queue_if_no_path = 1;
@@ -861,7 +878,7 @@ dm_suspend_and_flush_map (const char * mapname)
return 0;
}
condlog(2, "failed to remove multipath map %s", mapname);
- dm_simplecmd_noflush(DM_DEVICE_RESUME, mapname, 0);
+ dm_simplecmd_noflush(DM_DEVICE_RESUME, mapname, udev_flags);
if (queue_if_no_path)
s = dm_queue_if_no_path((char *)mapname, 1);
return 1;
@@ -1380,7 +1397,7 @@ rename_partmap (const char *name, void *data)
for (offset = strlen(rd->old); name[offset] && !(isdigit(name[offset])); offset++); /* do nothing */
snprintf(buff, PARAMS_SIZE, "%s%s%s", rd->new, rd->delim,
name + offset);
- dm_rename(name, buff, rd->delim);
+ dm_rename(name, buff, rd->delim, SKIP_KPARTX_OFF);
condlog(4, "partition map %s renamed", name);
return 0;
}
@@ -1403,11 +1420,12 @@ dm_rename_partmaps (const char * old, char * new, char *delim)
}
int
-dm_rename (const char * old, char * new, char *delim)
+dm_rename (const char * old, char * new, char *delim, int skip_kpartx)
{
int r = 0;
struct dm_task *dmt;
uint32_t cookie;
+ uint16_t udev_flags = DM_UDEV_DISABLE_LIBRARY_FALLBACK | ((skip_kpartx == SKIP_KPARTX_ON)? MPATH_UDEV_NO_KPARTX_FLAG : 0);
if (dm_rename_partmaps(old, new, delim))
return r;
@@ -1423,8 +1441,7 @@ dm_rename (const char * old, char * new, char *delim)
dm_task_no_open_count(dmt);
- if (!dm_task_set_cookie(dmt, &cookie,
- DM_UDEV_DISABLE_LIBRARY_FALLBACK))
+ if (!dm_task_set_cookie(dmt, &cookie, udev_flags))
goto out;
r = dm_task_run(dmt);
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 442d42e..e6d1090 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -12,6 +12,12 @@
#define MPATH_UDEV_RELOAD_FLAG 0
#endif
+#ifdef DM_SUBSYSTEM_UDEV_FLAG1
+#define MPATH_UDEV_NO_KPARTX_FLAG DM_SUBSYSTEM_UDEV_FLAG1
+#else
+#define MPATH_UDEV_NO_KPARTX_FLAG 0
+#endif
+
void dm_init(int verbosity);
int dm_prereq (void);
int dm_drv_version (unsigned int * version, char * str);
@@ -46,7 +52,7 @@ int dm_remove_partmaps (const char * mapname, int need_sync,
int deferred_remove);
int dm_get_uuid(char *name, char *uuid);
int dm_get_info (char * mapname, struct dm_info ** dmi);
-int dm_rename (const char * old, char * new, char * delim);
+int dm_rename (const char * old, char * new, char * delim, int skip_kpartx);
int dm_reassign(const char * mapname);
int dm_reassign_table(const char *name, char *old, char *new);
int dm_setgeometry(struct multipath *mpp);
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 7c21e72..e0a3014 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -403,6 +403,15 @@ declare_def_snprint(uev_wait_timeout, print_int)
declare_def_handler(strict_timing, set_yes_no)
declare_def_snprint(strict_timing, print_yes_no)
+declare_def_handler(skip_kpartx, set_yes_no_undef)
+declare_def_snprint_defint(skip_kpartx, print_yes_no_undef, YNU_NO)
+declare_ovr_handler(skip_kpartx, set_yes_no_undef)
+declare_ovr_snprint(skip_kpartx, print_yes_no_undef)
+declare_hw_handler(skip_kpartx, set_yes_no_undef)
+declare_hw_snprint(skip_kpartx, print_yes_no_undef)
+declare_mp_handler(skip_kpartx, set_yes_no_undef)
+declare_mp_snprint(skip_kpartx, print_yes_no_undef)
+
static int
def_config_dir_handler(struct config *conf, vector strvec)
{
@@ -1385,6 +1394,7 @@ init_keywords(vector keywords)
install_keyword("retrigger_tries", &def_retrigger_tries_handler, &snprint_def_retrigger_tries);
install_keyword("retrigger_delay", &def_retrigger_delay_handler, &snprint_def_retrigger_delay);
install_keyword("missing_uev_wait_timeout", &def_uev_wait_timeout_handler, &snprint_def_uev_wait_timeout);
+ install_keyword("skip_kpartx", &def_skip_kpartx_handler, &snprint_def_skip_kpartx);
__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);
@@ -1458,6 +1468,7 @@ init_keywords(vector keywords)
install_keyword("deferred_remove", &hw_deferred_remove_handler, &snprint_hw_deferred_remove);
install_keyword("delay_watch_checks", &hw_delay_watch_checks_handler, &snprint_hw_delay_watch_checks);
install_keyword("delay_wait_checks", &hw_delay_wait_checks_handler, &snprint_hw_delay_wait_checks);
+ install_keyword("skip_kpartx", &hw_skip_kpartx_handler, &snprint_hw_skip_kpartx);
install_sublevel_end();
install_keyword_root("overrides", &overrides_handler);
@@ -1485,6 +1496,7 @@ init_keywords(vector keywords)
install_keyword("deferred_remove", &ovr_deferred_remove_handler, &snprint_ovr_deferred_remove);
install_keyword("delay_watch_checks", &ovr_delay_watch_checks_handler, &snprint_ovr_delay_watch_checks);
install_keyword("delay_wait_checks", &ovr_delay_wait_checks_handler, &snprint_ovr_delay_wait_checks);
+ install_keyword("skip_kpartx", &ovr_skip_kpartx_handler, &snprint_ovr_skip_kpartx);
install_keyword_root("multipaths", &multipaths_handler);
install_keyword_multi("multipath", &multipath_handler, NULL);
@@ -1511,5 +1523,6 @@ init_keywords(vector keywords)
install_keyword("deferred_remove", &mp_deferred_remove_handler, &snprint_mp_deferred_remove);
install_keyword("delay_watch_checks", &mp_delay_watch_checks_handler, &snprint_mp_delay_watch_checks);
install_keyword("delay_wait_checks", &mp_delay_wait_checks_handler, &snprint_mp_delay_wait_checks);
+ install_keyword("skip_kpartx", &mp_skip_kpartx_handler, &snprint_mp_skip_kpartx);
install_sublevel_end();
}
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index aaf99fb..ec1fd92 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -665,4 +665,22 @@ out:
print_delay_checks(buff, 12, &mp->delay_wait_checks);
condlog(3, "%s: delay_wait_checks = %s %s", mp->alias, buff, origin);
return 0;
+
+}
+
+extern int
+select_skip_kpartx (struct config *conf, struct multipath * mp)
+{
+ char *origin;
+
+ mp_set_mpe(skip_kpartx);
+ mp_set_ovr(skip_kpartx);
+ mp_set_hwe(skip_kpartx);
+ mp_set_conf(skip_kpartx);
+ mp_set_default(skip_kpartx, DEFAULT_SKIP_KPARTX);
+out:
+ condlog(3, "%s: skip_kpartx = %s %s", mp->alias,
+ (mp->skip_kpartx == SKIP_KPARTX_ON)? "yes" : "no",
+ origin);
+ return 0;
}
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 5941a5f..3e6d607 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -22,3 +22,4 @@ int select_detect_prio(struct config *conf, struct path * pp);
int select_deferred_remove(struct config *conf, struct multipath *mp);
int select_delay_watch_checks (struct config *conf, struct multipath * mp);
int select_delay_wait_checks (struct config *conf, struct multipath * mp);
+int select_skip_kpartx (struct config *conf, struct multipath * mp);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index cb5d532..2078413 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -128,6 +128,12 @@ enum deferred_remove_states {
DEFERRED_REMOVE_IN_PROGRESS,
};
+enum skip_kpartx_states {
+ SKIP_KPARTX_UNDEF = YNU_UNDEF,
+ SKIP_KPARTX_OFF = YNU_NO,
+ SKIP_KPARTX_ON = YNU_YES,
+};
+
enum scsi_protocol {
SCSI_PROTOCOL_FCP = 0, /* Fibre Channel */
SCSI_PROTOCOL_SPI = 1, /* parallel SCSI */
@@ -243,6 +249,7 @@ struct multipath {
int deferred_remove;
int delay_watch_checks;
int delay_wait_checks;
+ int skip_kpartx;
unsigned int dev_loss;
uid_t uid;
gid_t gid;
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index eec319f..b7d7e59 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -842,6 +842,17 @@ Default value is: \fB30\fR
.RE
.
.
+.TP
+.B skip_kpartx
+If set to
+.I yes
+, kpartx will not automatically create partitions on the device.
+.RS
+.TP
+The default is \fBno\fR
+.RE
+.
+.
.\" ----------------------------------------------------------------------------
.SH "blacklist section"
.\" ----------------------------------------------------------------------------
@@ -972,6 +983,8 @@ are taken from the \fIdefaults\fR or \fIdevices\fR section:
.B delay_watch_checks
.TP
.B delay_wait_checks
+.TP
+.B skip_kpartx
.RE
.PD
.LP
@@ -1081,6 +1094,8 @@ section:
.B delay_watch_checks
.TP
.B delay_wait_checks
+.TP
+.B skip_kpartx
.RE
.PD
.LP
@@ -1141,6 +1156,8 @@ the values are taken from the \fIdevices\fR or \fIdefaults\fR sections:
.B delay_watch_checks
.TP
.B delay_wait_checks
+.TP
+.B skip_kpartx
.RE
.PD
.LP
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 8ff4362..181b2b8 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1078,19 +1078,21 @@ cli_resume(void * v, char ** reply, int * len, void * data)
char * param = get_keyparam(v, MAP);
int r;
struct multipath * mpp;
+ uint16_t udev_flags;
param = convert_dev(param, 0);
mpp = find_mp_by_alias(vecs->mpvec, param);
if (!mpp)
return 1;
+ udev_flags = (mpp->skip_kpartx)? MPATH_UDEV_NO_KPARTX_FLAG : 0;
if (mpp->wait_for_udev) {
condlog(2, "%s: device not fully created, failing resume",
mpp->alias);
return 1;
}
- r = dm_simplecmd_noflush(DM_DEVICE_RESUME, param, 0);
+ r = dm_simplecmd_noflush(DM_DEVICE_RESUME, param, udev_flags);
condlog(2, "%s: resume (operator)", param);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 02/10] kpartx.rules: respect skip_kpartx flag
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
2016-10-29 2:55 ` [PATCH 01/10] libmultipath: add skip_kpartx option Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:42 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 03/10] do not allow in-use path to change wwid Benjamin Marzinski
` (7 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
Check if DM_SUBSYSTEM_UDEV_FLAG1 is set, and if so, don't run kpartx.
If the event was not generated by device-mapper, just use the existing
value of DM_SUBSYSTEM_UDEV_FLAG1.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
kpartx/kpartx.rules | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index 1713f3c..48a4d6c 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -37,6 +37,8 @@ ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \
# Create dm tables for partitions
ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", GOTO="kpartx_end"
ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end"
+ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1"
+ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end"
ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \
RUN+="/sbin/kpartx -u -p -part /dev/$name"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 03/10] do not allow in-use path to change wwid
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
2016-10-29 2:55 ` [PATCH 01/10] libmultipath: add skip_kpartx option Benjamin Marzinski
2016-10-29 2:55 ` [PATCH 02/10] kpartx.rules: respect skip_kpartx flag Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:45 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 04/10] multipathd: add "map failures" format wildcard Benjamin Marzinski
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
When a path is part of a multipath device, it must not change it's wwid.
If it can, when multipathd is reconfigured, you can end up with two
multipath devices owning the same path, eventually leading to a crash.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/dmparser.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 9e79ecd..87e8398 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -380,6 +380,14 @@ disassemble_map (vector pathvec, char * params, struct multipath * mpp,
strncpy(pp->wwid, mpp->wwid,
WWID_SIZE - 1);
+ /*
+ * Do not allow in-use patch to change wwid
+ */
+ else if (strcmp(pp->wwid, mpp->wwid) != 0) {
+ condlog(0, "%s: path wwid appears to have changed. Using map wwid.\n", pp->dev_t);
+ strncpy(pp->wwid, mpp->wwid, WWID_SIZE);
+ }
+
pgp->id ^= (long)pp;
pp->pgindex = i + 1;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 04/10] multipathd: add "map failures" format wildcard
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
` (2 preceding siblings ...)
2016-10-29 2:55 ` [PATCH 03/10] do not allow in-use path to change wwid Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:45 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 05/10] mpath: don't wait for udev if all paths are down Benjamin Marzinski
` (5 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
This patch adds a new wildcard, 'x', for the "show maps format" command.
This wildcard show the number of map failures that have occurred. A map
failure is any time that the multipath device enters a state where it
has no paths and is not set to queue_if_no_paths. It can be used to see
if a multipath device was ever in a state were it could fail IO errors
up.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/print.c | 7 +++++++
libmultipath/structs.h | 1 +
libmultipath/structs_vec.c | 28 ++++++++++++++++------------
multipathd/cli_handlers.c | 11 ++++++++++-
multipathd/main.c | 2 ++
5 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 9aa41ad..f626dc5 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -245,6 +245,12 @@ snprint_q_timeouts (char * buff, size_t len, struct multipath * mpp)
}
static int
+snprint_map_failures (char * buff, size_t len, struct multipath * mpp)
+{
+ return snprint_uint(buff, len, mpp->stat_map_failures);
+}
+
+static int
snprint_multipath_uuid (char * buff, size_t len, struct multipath * mpp)
{
return snprint_str(buff, len, mpp->wwid);
@@ -619,6 +625,7 @@ struct multipath_data mpd[] = {
{'t', "dm-st", 0, snprint_dm_map_state},
{'S', "size", 0, snprint_multipath_size},
{'f', "features", 0, snprint_features},
+ {'x', "failures", 0, snprint_map_failures},
{'h', "hwhandler", 0, snprint_hwhandler},
{'A', "action", 0, snprint_action},
{'0', "path_faults", 0, snprint_path_faults},
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 2078413..3a716d8 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -277,6 +277,7 @@ struct multipath {
unsigned int stat_map_loads;
unsigned int stat_total_queueing_time;
unsigned int stat_queueing_timeouts;
+ unsigned int stat_map_failures;
/* checkers shared data */
void * mpcontext;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index a0c8869..e898528 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -610,19 +610,23 @@ int update_multipath (struct vectors *vecs, char *mapname, int reset)
*/
void update_queue_mode_del_path(struct multipath *mpp)
{
- if (--mpp->nr_active == 0 && mpp->no_path_retry > 0) {
- struct config *conf = get_multipath_config();
+ if (--mpp->nr_active == 0) {
+ if (mpp->no_path_retry > 0) {
+ struct config *conf = get_multipath_config();
- /*
- * Enter retry mode.
- * meaning of +1: retry_tick may be decremented in
- * checkerloop before starting retry.
- */
- mpp->stat_queueing_timeouts++;
- mpp->retry_tick = mpp->no_path_retry * conf->checkint + 1;
- condlog(1, "%s: Entering recovery mode: max_retries=%d",
- mpp->alias, mpp->no_path_retry);
- put_multipath_config(conf);
+ /*
+ * Enter retry mode.
+ * meaning of +1: retry_tick may be decremented in
+ * checkerloop before starting retry.
+ */
+ mpp->stat_queueing_timeouts++;
+ mpp->retry_tick = mpp->no_path_retry *
+ conf->checkint + 1;
+ condlog(1, "%s: Entering recovery mode: max_retries=%d",
+ mpp->alias, mpp->no_path_retry);
+ put_multipath_config(conf);
+ } else if (mpp->no_path_retry != NO_PATH_RETRY_QUEUE)
+ mpp->stat_map_failures++;
}
condlog(2, "%s: remaining active paths: %d", mpp->alias, mpp->nr_active);
}
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 181b2b8..b0eeca6 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -498,9 +498,14 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style,
c += snprint_multipath_header(c, reply + maxlen - c,
style);
- vector_foreach_slot(vecs->mpvec, mpp, i)
+ vector_foreach_slot(vecs->mpvec, mpp, i) {
+ if (update_multipath(vecs, mpp->alias, 0)) {
+ i--;
+ continue;
+ }
c += snprint_multipath(c, reply + maxlen - c,
style, mpp, pretty);
+ }
again = ((c - reply) == (maxlen - 1));
@@ -997,6 +1002,8 @@ cli_disable_queueing(void *v, char **reply, int *len, void *data)
return 1;
}
+ if (mpp->nr_active == 0)
+ mpp->stat_map_failures++;
mpp->retry_tick = -1;
dm_queue_if_no_path(mpp->alias, 0);
return 0;
@@ -1011,6 +1018,8 @@ cli_disable_all_queueing(void *v, char **reply, int *len, void *data)
condlog(2, "disable queueing (operator)");
vector_foreach_slot(vecs->mpvec, mpp, i) {
+ if (mpp->nr_active == 0)
+ mpp->stat_map_failures++;
mpp->retry_tick = -1;
dm_queue_if_no_path(mpp->alias, 0);
}
diff --git a/multipathd/main.c b/multipathd/main.c
index 03c2dd9..dbcaa03 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -889,6 +889,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs)
mpp->retry_tick = 0;
mpp->no_path_retry = NO_PATH_RETRY_FAIL;
mpp->flush_on_last_del = FLUSH_IN_PROGRESS;
+ mpp->stat_map_failures++;
dm_queue_if_no_path(mpp->alias, 0);
}
if (!flush_map(mpp, vecs, 1)) {
@@ -1397,6 +1398,7 @@ retry_count_tick(vector mpvec)
mpp->stat_total_queueing_time++;
condlog(4, "%s: Retrying.. No active path", mpp->alias);
if(--mpp->retry_tick == 0) {
+ mpp->stat_map_failures++;
dm_queue_if_no_path(mpp->alias, 0);
condlog(2, "%s: Disable queueing", mpp->alias);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 05/10] mpath: don't wait for udev if all paths are down
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
` (3 preceding siblings ...)
2016-10-29 2:55 ` [PATCH 04/10] multipathd: add "map failures" format wildcard Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:46 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 06/10] multipath: set cookie before using it Benjamin Marzinski
` (4 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
Normally multipath waits for udev to create a device before adding
more paths, which could trigger a reload. But, if the first path
discovered is not usable, you should add the next path right away.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
multipathd/main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index dbcaa03..15c957a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -715,7 +715,10 @@ ev_add_path (struct path * pp, struct vectors * vecs)
goto fail; /* leave path added to pathvec */
}
mpp = find_mp_by_wwid(vecs->mpvec, pp->wwid);
- if (mpp && mpp->wait_for_udev) {
+ if (mpp && mpp->wait_for_udev &&
+ (pathcount(mpp, PATH_UP) > 0 ||
+ (pathcount(mpp, PATH_GHOST) > 0 && pp->tpgs != TPGS_IMPLICIT))) {
+ /* if wait_for_udev is set and valid paths exist */
mpp->wait_for_udev = 2;
orphan_path(pp, "waiting for create to complete");
return 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 06/10] multipath: set cookie before using it.
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
` (4 preceding siblings ...)
2016-10-29 2:55 ` [PATCH 05/10] mpath: don't wait for udev if all paths are down Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:46 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 07/10] recover from errors in multipathd startup Benjamin Marzinski
` (3 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
dm_task_set_cookie() expects the cookie to be initialized when it is
called, but dm_rename() didn't initialized it, causing renames to fail
some of the time.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/devmapper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index ee61ff0..546e227 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1424,7 +1424,7 @@ dm_rename (const char * old, char * new, char *delim, int skip_kpartx)
{
int r = 0;
struct dm_task *dmt;
- uint32_t cookie;
+ uint32_t cookie = 0;
uint16_t udev_flags = DM_UDEV_DISABLE_LIBRARY_FALLBACK | ((skip_kpartx == SKIP_KPARTX_ON)? MPATH_UDEV_NO_KPARTX_FLAG : 0);
if (dm_rename_partmaps(old, new, delim))
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 07/10] recover from errors in multipathd startup
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
` (5 preceding siblings ...)
2016-10-29 2:55 ` [PATCH 06/10] multipath: set cookie before using it Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:47 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 08/10] fix INIT_REQUESTED_UDEV code Benjamin Marzinski
` (2 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
When multipathd does it's initial configuration during startup, it fails
the daemon for many errors that it would simply recover from if they
occured when adding paths or maps later. It should recover from these
errors during startup as well. Also, if multipathd hits a
nonrecoverable error, it should log a message before quitting.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/configure.c | 8 +++++---
multipathd/main.c | 45 ++++++++++++++++++++++++++++++++-------------
2 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 48f100b..d428099 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -809,8 +809,10 @@ coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid, int force_r
* at this point, we know we really got a new mp
*/
mpp = add_map_with_path(vecs, pp1, 0);
- if (!mpp)
- return 1;
+ if (!mpp) {
+ orphan_path(pp1, "failed to create multipath device");
+ continue;
+ }
if (pp1->priority == PRIO_UNDEF)
mpp->action = ACT_REJECT;
@@ -862,7 +864,7 @@ coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid, int force_r
condlog(3, "%s: domap (%u) failure "
"for create/reload map",
mpp->alias, r);
- if (r == DOMAP_FAIL) {
+ if (r == DOMAP_FAIL || is_daemon) {
condlog(2, "%s: %s map",
mpp->alias, (mpp->action == ACT_CREATE)?
"ignoring" : "removing");
diff --git a/multipathd/main.c b/multipathd/main.c
index 15c957a..f423e35 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1017,7 +1017,7 @@ map_discovery (struct vectors * vecs)
vector_foreach_slot (vecs->mpvec, mpp, i)
if (setup_multipath(vecs, mpp))
- return 1;
+ i--;
return 0;
}
@@ -1910,21 +1910,29 @@ configure (struct vectors * vecs, int start_waiters)
int i, ret;
struct config *conf;
- if (!vecs->pathvec && !(vecs->pathvec = vector_alloc()))
+ if (!vecs->pathvec && !(vecs->pathvec = vector_alloc())) {
+ condlog(0, "couldn't allocate path vec in configure");
return 1;
+ }
- if (!vecs->mpvec && !(vecs->mpvec = vector_alloc()))
+ if (!vecs->mpvec && !(vecs->mpvec = vector_alloc())) {
+ condlog(0, "couldn't allocate multipath vec in configure");
return 1;
+ }
- if (!(mpvec = vector_alloc()))
+ if (!(mpvec = vector_alloc())) {
+ condlog(0, "couldn't allocate new maps vec in configure");
return 1;
+ }
/*
* probe for current path (from sysfs) and map (from dm) sets
*/
ret = path_discovery(vecs->pathvec, DI_ALL);
- if (ret < 0)
+ if (ret < 0) {
+ condlog(0, "configure failed at path discovery");
return 1;
+ }
vector_foreach_slot (vecs->pathvec, pp, i){
conf = get_multipath_config();
@@ -1937,21 +1945,27 @@ configure (struct vectors * vecs, int start_waiters)
pp->checkint = conf->checkint;
put_multipath_config(conf);
}
- if (map_discovery(vecs))
+ if (map_discovery(vecs)) {
+ condlog(0, "configure failed at map discovery");
return 1;
+ }
/*
* create new set of maps & push changed ones into dm
*/
- if (coalesce_paths(vecs, mpvec, NULL, 1, CMD_NONE))
+ if (coalesce_paths(vecs, mpvec, NULL, 1, CMD_NONE)) {
+ condlog(0, "configure failed while coalescing paths");
return 1;
+ }
/*
* may need to remove some maps which are no longer relevant
* e.g., due to blacklist changes in conf file
*/
- if (coalesce_maps(vecs, mpvec))
+ if (coalesce_maps(vecs, mpvec)) {
+ condlog(0, "configure failed while coalescing maps");
return 1;
+ }
dm_lib_release();
@@ -1976,11 +1990,16 @@ configure (struct vectors * vecs, int start_waiters)
* start dm event waiter threads for these new maps
*/
vector_foreach_slot(vecs->mpvec, mpp, i) {
- if (setup_multipath(vecs, mpp))
- return 1;
- if (start_waiters)
- if (start_waiter_thread(mpp, vecs))
- return 1;
+ if (setup_multipath(vecs, mpp)) {
+ i--;
+ continue;
+ }
+ if (start_waiters) {
+ if (start_waiter_thread(mpp, vecs)) {
+ remove_map(mpp, vecs, 1);
+ i--;
+ }
+ }
}
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 08/10] fix INIT_REQUESTED_UDEV code
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
` (6 preceding siblings ...)
2016-10-29 2:55 ` [PATCH 07/10] recover from errors in multipathd startup Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:48 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 09/10] add disable_changed_wwids option Benjamin Marzinski
2016-10-29 2:55 ` [PATCH 10/10] set retrigger_tries to 0 for multipath Benjamin Marzinski
9 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
uev_update_path was not checking pp->initialized to see if multipathd
had requested that the path be reinitialized unless the path's read-only
state had changed. This kept the reinitialization code from running in
most cases where it was supposed to. This patch reorders the function to
move that check outside the read-only status change code block. This
does mean that uev_update_path now always grabs the vecs lock, where
before it would only be grabbed if the read-only status had changed. If
people are worried about this, I can add some code to limit this so that
uev_update_path will only grab it if there is a chance that it needs to
reinitialize the path.
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
multipathd/main.c | 54 +++++++++++++++++++-----------------------------------
1 file changed, 19 insertions(+), 35 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index f423e35..dbb4554 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -957,51 +957,35 @@ static int
uev_update_path (struct uevent *uev, struct vectors * vecs)
{
int ro, retval = 0;
+ struct path * pp;
ro = uevent_get_disk_ro(uev);
- if (ro >= 0) {
- struct path * pp;
- struct multipath *mpp = NULL;
+ pthread_cleanup_push(cleanup_lock, &vecs->lock);
+ lock(&vecs->lock);
+ pthread_testcancel();
- condlog(2, "%s: update path write_protect to '%d' (uevent)",
- uev->kernel, ro);
- pthread_cleanup_push(cleanup_lock, &vecs->lock);
- lock(&vecs->lock);
- pthread_testcancel();
- /*
- * pthread_mutex_lock() and pthread_mutex_unlock()
- * need to be at the same indentation level, hence
- * this slightly convoluted codepath.
- */
- pp = find_path_by_dev(vecs->pathvec, uev->kernel);
- if (pp) {
- if (pp->initialized == INIT_REQUESTED_UDEV) {
- retval = 2;
- } else {
- mpp = pp->mpp;
- if (mpp && mpp->wait_for_udev) {
- mpp->wait_for_udev = 2;
- mpp = NULL;
- retval = 0;
- }
- }
- if (mpp) {
- retval = reload_map(vecs, mpp, 0, 1);
+ pp = find_path_by_dev(vecs->pathvec, uev->kernel);
+ if (pp) {
+ struct multipath *mpp = pp->mpp;
+
+ if (pp->initialized == INIT_REQUESTED_UDEV)
+ retval = uev_add_path(uev, vecs);
+ else if (mpp && ro >= 0) {
+ condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
+ if (mpp->wait_for_udev)
+ mpp->wait_for_udev = 2;
+ else {
+ retval = reload_map(vecs, mpp, 0, 1);
condlog(2, "%s: map %s reloaded (retval %d)",
uev->kernel, mpp->alias, retval);
}
}
- lock_cleanup_pop(vecs->lock);
- if (!pp) {
- condlog(0, "%s: spurious uevent, path not found",
- uev->kernel);
- return 1;
- }
- if (retval == 2)
- return uev_add_path(uev, vecs);
}
+ lock_cleanup_pop(vecs->lock);
+ if (!pp)
+ condlog(0, "%s: spurious uevent, path not found", uev->kernel);
return retval;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 09/10] add disable_changed_wwids option
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
` (7 preceding siblings ...)
2016-10-29 2:55 ` [PATCH 08/10] fix INIT_REQUESTED_UDEV code Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:54 ` Hannes Reinecke
2016-11-06 0:03 ` Xose Vazquez Perez
2016-10-29 2:55 ` [PATCH 10/10] set retrigger_tries to 0 for multipath Benjamin Marzinski
9 siblings, 2 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
If a LUN on a storage device gets remapped while in-use by multipath,
it's possible that the multipath device will continue writing to this
new LUN, causing corruption. This is not multipath's fault (users
should go remapping in-use LUNs), but it's possible for multipath to
detect this and disable IO to the device. If disable_changed_wwids
is set to "yes", multipathd will detect when a LUN changes wwids when it
receives the uevent for this, and will disable access to the device
until the LUN is mapped back.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/config.c | 1 +
libmultipath/config.h | 1 +
libmultipath/defaults.h | 1 +
libmultipath/dict.c | 4 ++++
libmultipath/discovery.c | 15 +++++++--------
libmultipath/discovery.h | 1 +
libmultipath/structs.h | 1 +
multipathd/main.c | 32 ++++++++++++++++++++++++++++++++
8 files changed, 48 insertions(+), 8 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index bdcad80..a97b318 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -618,6 +618,7 @@ load_config (char * file)
conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT;
conf->deferred_remove = DEFAULT_DEFERRED_REMOVE;
conf->skip_kpartx = DEFAULT_SKIP_KPARTX;
+ conf->disable_changed_wwids = DEFAULT_DISABLE_CHANGED_WWIDS;
/*
* preload default hwtable
diff --git a/libmultipath/config.h b/libmultipath/config.h
index d59a993..dbdaa44 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -144,6 +144,7 @@ struct config {
int delayed_reconfig;
int uev_wait_timeout;
int skip_kpartx;
+ int disable_changed_wwids;
unsigned int version[3];
char * multipath_dir;
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index a1fee9b..a72078f 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -36,6 +36,7 @@
#define DEFAULT_FORCE_SYNC 0
#define DEFAULT_PARTITION_DELIM NULL
#define DEFAULT_SKIP_KPARTX SKIP_KPARTX_OFF
+#define DEFAULT_DISABLE_CHANGED_WWIDS 0
#define DEFAULT_CHECKINT 5
#define MAX_CHECKINT(a) (a << 2)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index e0a3014..61b6910 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -412,6 +412,9 @@ declare_hw_snprint(skip_kpartx, print_yes_no_undef)
declare_mp_handler(skip_kpartx, set_yes_no_undef)
declare_mp_snprint(skip_kpartx, print_yes_no_undef)
+declare_def_handler(disable_changed_wwids, set_yes_no)
+declare_def_snprint(disable_changed_wwids, print_yes_no)
+
static int
def_config_dir_handler(struct config *conf, vector strvec)
{
@@ -1395,6 +1398,7 @@ init_keywords(vector keywords)
install_keyword("retrigger_delay", &def_retrigger_delay_handler, &snprint_def_retrigger_delay);
install_keyword("missing_uev_wait_timeout", &def_uev_wait_timeout_handler, &snprint_def_uev_wait_timeout);
install_keyword("skip_kpartx", &def_skip_kpartx_handler, &snprint_def_skip_kpartx);
+ install_keyword("disable_changed_wwids", &def_disable_changed_wwids_handler, &snprint_def_disable_changed_wwids);
__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/discovery.c b/libmultipath/discovery.c
index bb3116d..756344f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1538,13 +1538,12 @@ get_prio (struct path * pp)
}
static int
-get_udev_uid(struct path * pp, char *uid_attribute)
+get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
{
ssize_t len;
const char *value;
- value = udev_device_get_property_value(pp->udev,
- uid_attribute);
+ value = udev_device_get_property_value(udev, uid_attribute);
if (!value || strlen(value) == 0)
value = getenv(uid_attribute);
if (value && strlen(value)) {
@@ -1625,8 +1624,8 @@ get_vpd_uid(struct path * pp)
return get_vpd_sysfs(parent, 0x83, pp->wwid, WWID_SIZE);
}
-static int
-get_uid (struct path * pp, int path_state)
+int
+get_uid (struct path * pp, int path_state, struct udev_device *udev)
{
char *c;
const char *origin = "unknown";
@@ -1639,7 +1638,7 @@ get_uid (struct path * pp, int path_state)
put_multipath_config(conf);
}
- if (!pp->udev) {
+ if (!udev) {
condlog(1, "%s: no udev information", pp->dev);
return 1;
}
@@ -1669,7 +1668,7 @@ get_uid (struct path * pp, int path_state)
int retrigger;
if (pp->uid_attribute) {
- len = get_udev_uid(pp, pp->uid_attribute);
+ len = get_udev_uid(pp, pp->uid_attribute, udev);
origin = "udev";
if (len <= 0)
condlog(1,
@@ -1798,7 +1797,7 @@ pathinfo (struct path *pp, struct config *conf, int mask)
}
if ((mask & DI_WWID) && !strlen(pp->wwid)) {
- get_uid(pp, path_state);
+ get_uid(pp, path_state, pp->udev);
if (!strlen(pp->wwid)) {
pp->initialized = INIT_MISSING_UDEV;
pp->tick = conf->retrigger_delay;
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index 0f5b1e6..176eac1 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -49,6 +49,7 @@ ssize_t sysfs_get_vpd (struct udev_device * udev, int pg, unsigned char * buff,
size_t len);
int sysfs_get_asymmetric_access_state(struct path *pp,
char *buff, int buflen);
+int get_uid(struct path * pp, int path_state, struct udev_device *udev);
/*
* discovery bitmask
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 3a716d8..58508f6 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -217,6 +217,7 @@ struct path {
int fd;
int initialized;
int retriggers;
+ int wwid_changed;
/* configlet pointers */
struct hwentry * hwe;
diff --git a/multipathd/main.c b/multipathd/main.c
index dbb4554..bb96cca 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -958,6 +958,12 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
{
int ro, retval = 0;
struct path * pp;
+ struct config *conf;
+ int disable_changed_wwids;
+
+ conf = get_multipath_config();
+ disable_changed_wwids = conf->disable_changed_wwids;
+ put_multipath_config(conf);
ro = uevent_get_disk_ro(uev);
@@ -969,6 +975,25 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
if (pp) {
struct multipath *mpp = pp->mpp;
+ if (disable_changed_wwids &&
+ (strlen(pp->wwid) || pp->wwid_changed)) {
+ char wwid[WWID_SIZE];
+
+ strcpy(wwid, pp->wwid);
+ get_uid(pp, pp->state, uev->udev);
+ if (strcmp(wwid, pp->wwid) != 0) {
+ condlog(0, "%s: path wwid changed from '%s' to '%s'. disallowing", uev->kernel, wwid, pp->wwid);
+ strcpy(pp->wwid, wwid);
+ if (!pp->wwid_changed) {
+ pp->wwid_changed = 1;
+ pp->tick = 1;
+ dm_fail_path(pp->mpp->alias, pp->dev_t);
+ }
+ goto out;
+ } else
+ pp->wwid_changed = 0;
+ }
+
if (pp->initialized == INIT_REQUESTED_UDEV)
retval = uev_add_path(uev, vecs);
else if (mpp && ro >= 0) {
@@ -983,6 +1008,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
}
}
}
+out:
lock_cleanup_pop(vecs->lock);
if (!pp)
condlog(0, "%s: spurious uevent, path not found", uev->kernel);
@@ -1509,6 +1535,12 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
} else
checker_clear_message(&pp->checker);
+ if (pp->wwid_changed) {
+ condlog(2, "%s: path wwid has changed. Refusing to use",
+ pp->dev);
+ newstate = PATH_DOWN;
+ }
+
if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
condlog(2, "%s: unusable path", pp->dev);
conf = get_multipath_config();
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 10/10] set retrigger_tries to 0 for multipath
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
` (8 preceding siblings ...)
2016-10-29 2:55 ` [PATCH 09/10] add disable_changed_wwids option Benjamin Marzinski
@ 2016-10-29 2:55 ` Benjamin Marzinski
2016-10-30 13:54 ` Hannes Reinecke
2016-12-06 15:11 ` Xose Vazquez Perez
9 siblings, 2 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-29 2:55 UTC (permalink / raw)
To: device-mapper development
Multipathd uses retrigger_tries to give udev more chances to to fill in
the uid_attribute, so that the path device is correctly set up in the
udev database. However the multipath command can't do this, so it should
just immediately give up on udev, and try to get the wwid directly.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
multipath/main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/multipath/main.c b/multipath/main.c
index ee00fdb..06add30 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -521,6 +521,7 @@ main (int argc, char *argv[])
if (!conf)
exit(1);
multipath_conf = conf;
+ conf->retrigger_tries = 0;
while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BritquwW")) != EOF ) {
switch(arg) {
case 1: printf("optarg : %s\n",optarg);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 01/10] libmultipath: add skip_kpartx option
2016-10-29 2:55 ` [PATCH 01/10] libmultipath: add skip_kpartx option Benjamin Marzinski
@ 2016-10-30 13:42 ` Hannes Reinecke
0 siblings, 0 replies; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:42 UTC (permalink / raw)
To: dm-devel
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> This option gives multipath the ability to stop kpartx from running. The
> previous idea, the "no_partitions" feature, was not accepted in the
> upstream kernel. This method uses one of the dm cookie subsystem flags
> DM_SUBSYSTEM_UDEV_FLAG1, which can be checked by udev to skip running
> kpartx when processing the event. This patch does most of the work
> necessary to make this work. It doesn't change kpartx.rules, however.
>
> Also, if dm_suspend_and_flush_map fails, multipath doesn't know how the
> device is configured. so, it simply checks if the device has any
> partitions before attempting the remove, and if not, sets the
> DM_SUBSYSTEM_UDEV_FLAG1 on the resume after failure, so that no
> partitions will be generated.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/config.c | 2 ++
> libmultipath/config.h | 3 +++
> libmultipath/configure.c | 5 +++--
> libmultipath/defaults.h | 1 +
> libmultipath/devmapper.c | 45 +++++++++++++++++++++++++++++++--------------
> libmultipath/devmapper.h | 8 +++++++-
> libmultipath/dict.c | 13 +++++++++++++
> libmultipath/propsel.c | 18 ++++++++++++++++++
> libmultipath/propsel.h | 1 +
> libmultipath/structs.h | 7 +++++++
> multipath/multipath.conf.5 | 17 +++++++++++++++++
> multipathd/cli_handlers.c | 4 +++-
> 12 files changed, 106 insertions(+), 18 deletions(-)
>
Thank you for this.
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 02/10] kpartx.rules: respect skip_kpartx flag
2016-10-29 2:55 ` [PATCH 02/10] kpartx.rules: respect skip_kpartx flag Benjamin Marzinski
@ 2016-10-30 13:42 ` Hannes Reinecke
0 siblings, 0 replies; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:42 UTC (permalink / raw)
To: dm-devel
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> Check if DM_SUBSYSTEM_UDEV_FLAG1 is set, and if so, don't run kpartx.
> If the event was not generated by device-mapper, just use the existing
> value of DM_SUBSYSTEM_UDEV_FLAG1.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> kpartx/kpartx.rules | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
> index 1713f3c..48a4d6c 100644
> --- a/kpartx/kpartx.rules
> +++ b/kpartx/kpartx.rules
> @@ -37,6 +37,8 @@ ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \
> # Create dm tables for partitions
> ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", GOTO="kpartx_end"
> ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end"
> +ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1"
> +ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end"
> ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \
> RUN+="/sbin/kpartx -u -p -part /dev/$name"
>
>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] do not allow in-use path to change wwid
2016-10-29 2:55 ` [PATCH 03/10] do not allow in-use path to change wwid Benjamin Marzinski
@ 2016-10-30 13:45 ` Hannes Reinecke
2016-10-31 14:30 ` Benjamin Marzinski
0 siblings, 1 reply; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:45 UTC (permalink / raw)
To: dm-devel, Benjamin Marzinski
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> When a path is part of a multipath device, it must not change it's wwid.
> If it can, when multipathd is reconfigured, you can end up with two
> multipath devices owning the same path, eventually leading to a crash.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/dmparser.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
Hmm. While I do see that this is an issue, just continuing is probably
as bad; the wwid change might be genuine, in which case this device has
no business being part of that particular multipath device.
Can't we just evict that offending path eg by orphaning it and let the
admin figure things out?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 04/10] multipathd: add "map failures" format wildcard
2016-10-29 2:55 ` [PATCH 04/10] multipathd: add "map failures" format wildcard Benjamin Marzinski
@ 2016-10-30 13:45 ` Hannes Reinecke
0 siblings, 0 replies; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:45 UTC (permalink / raw)
To: dm-devel
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> This patch adds a new wildcard, 'x', for the "show maps format" command.
> This wildcard show the number of map failures that have occurred. A map
> failure is any time that the multipath device enters a state where it
> has no paths and is not set to queue_if_no_paths. It can be used to see
> if a multipath device was ever in a state were it could fail IO errors
> up.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/print.c | 7 +++++++
> libmultipath/structs.h | 1 +
> libmultipath/structs_vec.c | 28 ++++++++++++++++------------
> multipathd/cli_handlers.c | 11 ++++++++++-
> multipathd/main.c | 2 ++
> 5 files changed, 36 insertions(+), 13 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 05/10] mpath: don't wait for udev if all paths are down
2016-10-29 2:55 ` [PATCH 05/10] mpath: don't wait for udev if all paths are down Benjamin Marzinski
@ 2016-10-30 13:46 ` Hannes Reinecke
0 siblings, 0 replies; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:46 UTC (permalink / raw)
To: dm-devel
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> Normally multipath waits for udev to create a device before adding
> more paths, which could trigger a reload. But, if the first path
> discovered is not usable, you should add the next path right away.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> multipathd/main.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index dbcaa03..15c957a 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -715,7 +715,10 @@ ev_add_path (struct path * pp, struct vectors * vecs)
> goto fail; /* leave path added to pathvec */
> }
> mpp = find_mp_by_wwid(vecs->mpvec, pp->wwid);
> - if (mpp && mpp->wait_for_udev) {
> + if (mpp && mpp->wait_for_udev &&
> + (pathcount(mpp, PATH_UP) > 0 ||
> + (pathcount(mpp, PATH_GHOST) > 0 && pp->tpgs != TPGS_IMPLICIT))) {
> + /* if wait_for_udev is set and valid paths exist */
> mpp->wait_for_udev = 2;
> orphan_path(pp, "waiting for create to complete");
> return 0;
>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] multipath: set cookie before using it.
2016-10-29 2:55 ` [PATCH 06/10] multipath: set cookie before using it Benjamin Marzinski
@ 2016-10-30 13:46 ` Hannes Reinecke
0 siblings, 0 replies; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:46 UTC (permalink / raw)
To: dm-devel
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> dm_task_set_cookie() expects the cookie to be initialized when it is
> called, but dm_rename() didn't initialized it, causing renames to fail
> some of the time.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/devmapper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index ee61ff0..546e227 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -1424,7 +1424,7 @@ dm_rename (const char * old, char * new, char *delim, int skip_kpartx)
> {
> int r = 0;
> struct dm_task *dmt;
> - uint32_t cookie;
> + uint32_t cookie = 0;
> uint16_t udev_flags = DM_UDEV_DISABLE_LIBRARY_FALLBACK | ((skip_kpartx == SKIP_KPARTX_ON)? MPATH_UDEV_NO_KPARTX_FLAG : 0);
>
> if (dm_rename_partmaps(old, new, delim))
>
We never get this 'cookie' business right ...
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] recover from errors in multipathd startup
2016-10-29 2:55 ` [PATCH 07/10] recover from errors in multipathd startup Benjamin Marzinski
@ 2016-10-30 13:47 ` Hannes Reinecke
0 siblings, 0 replies; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:47 UTC (permalink / raw)
To: dm-devel
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> When multipathd does it's initial configuration during startup, it fails
> the daemon for many errors that it would simply recover from if they
> occured when adding paths or maps later. It should recover from these
> errors during startup as well. Also, if multipathd hits a
> nonrecoverable error, it should log a message before quitting.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/configure.c | 8 +++++---
> multipathd/main.c | 45 ++++++++++++++++++++++++++++++++-------------
> 2 files changed, 37 insertions(+), 16 deletions(-)
>
Very nice.
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] fix INIT_REQUESTED_UDEV code
2016-10-29 2:55 ` [PATCH 08/10] fix INIT_REQUESTED_UDEV code Benjamin Marzinski
@ 2016-10-30 13:48 ` Hannes Reinecke
0 siblings, 0 replies; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:48 UTC (permalink / raw)
To: Benjamin Marzinski, device-mapper development
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> uev_update_path was not checking pp->initialized to see if multipathd
> had requested that the path be reinitialized unless the path's read-only
> state had changed. This kept the reinitialization code from running in
> most cases where it was supposed to. This patch reorders the function to
> move that check outside the read-only status change code block. This
> does mean that uev_update_path now always grabs the vecs lock, where
> before it would only be grabbed if the read-only status had changed. If
> people are worried about this, I can add some code to limit this so that
> uev_update_path will only grab it if there is a chance that it needs to
> reinitialize the path.
>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> multipathd/main.c | 54 +++++++++++++++++++-----------------------------------
> 1 file changed, 19 insertions(+), 35 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 09/10] add disable_changed_wwids option
2016-10-29 2:55 ` [PATCH 09/10] add disable_changed_wwids option Benjamin Marzinski
@ 2016-10-30 13:54 ` Hannes Reinecke
2016-11-04 15:32 ` Benjamin Marzinski
2016-11-06 0:03 ` Xose Vazquez Perez
1 sibling, 1 reply; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:54 UTC (permalink / raw)
To: dm-devel
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> If a LUN on a storage device gets remapped while in-use by multipath,
> it's possible that the multipath device will continue writing to this
> new LUN, causing corruption. This is not multipath's fault (users
> should go remapping in-use LUNs), but it's possible for multipath to
> detect this and disable IO to the device. If disable_changed_wwids
> is set to "yes", multipathd will detect when a LUN changes wwids when it
> receives the uevent for this, and will disable access to the device
> until the LUN is mapped back.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/config.c | 1 +
> libmultipath/config.h | 1 +
> libmultipath/defaults.h | 1 +
> libmultipath/dict.c | 4 ++++
> libmultipath/discovery.c | 15 +++++++--------
> libmultipath/discovery.h | 1 +
> libmultipath/structs.h | 1 +
> multipathd/main.c | 32 ++++++++++++++++++++++++++++++++
> 8 files changed, 48 insertions(+), 8 deletions(-)
>
Hmm. Not sure if the really buys us anything. By the time we process the
uevent it might already be too late, and I/O might already have been
written to that device.
I do agree on the warning, though.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 10/10] set retrigger_tries to 0 for multipath
2016-10-29 2:55 ` [PATCH 10/10] set retrigger_tries to 0 for multipath Benjamin Marzinski
@ 2016-10-30 13:54 ` Hannes Reinecke
2016-12-06 15:11 ` Xose Vazquez Perez
1 sibling, 0 replies; 29+ messages in thread
From: Hannes Reinecke @ 2016-10-30 13:54 UTC (permalink / raw)
To: dm-devel
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> Multipathd uses retrigger_tries to give udev more chances to to fill in
> the uid_attribute, so that the path device is correctly set up in the
> udev database. However the multipath command can't do this, so it should
> just immediately give up on udev, and try to get the wwid directly.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> multipath/main.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/multipath/main.c b/multipath/main.c
> index ee00fdb..06add30 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -521,6 +521,7 @@ main (int argc, char *argv[])
> if (!conf)
> exit(1);
> multipath_conf = conf;
> + conf->retrigger_tries = 0;
> while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BritquwW")) != EOF ) {
> switch(arg) {
> case 1: printf("optarg : %s\n",optarg);
>
Hehe.
I've come across the very same issue.
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] do not allow in-use path to change wwid
2016-10-30 13:45 ` Hannes Reinecke
@ 2016-10-31 14:30 ` Benjamin Marzinski
0 siblings, 0 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2016-10-31 14:30 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: dm-devel
On Sun, Oct 30, 2016 at 02:45:01PM +0100, Hannes Reinecke wrote:
> On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> >When a path is part of a multipath device, it must not change it's wwid.
> >If it can, when multipathd is reconfigured, you can end up with two
> >multipath devices owning the same path, eventually leading to a crash.
> >
> >Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> >---
> > libmultipath/dmparser.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> Hmm. While I do see that this is an issue, just continuing is probably as
> bad; the wwid change might be genuine, in which case this device has no
> business being part of that particular multipath device.
> Can't we just evict that offending path eg by orphaning it and let the admin
> figure things out?
Possibly, but sometimes devices change wwids temporarily when they get
temporarily unmapped, which can happen when you resize them. When I
tried orphaning them, I could get multipath devices getting created for
that temporary wwid, which was pretty confusing. My later patch also
disables access to these paths, so multipath can't keep writing to them,
but you don't get these annoying fake mutipath devices.
-Ben
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke zSeries & Storage
> hare@suse.de +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 09/10] add disable_changed_wwids option
2016-10-30 13:54 ` Hannes Reinecke
@ 2016-11-04 15:32 ` Benjamin Marzinski
2017-02-28 12:27 ` Zhangguanghui
0 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-11-04 15:32 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: dm-devel
On Sun, Oct 30, 2016 at 02:54:14PM +0100, Hannes Reinecke wrote:
> On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> >If a LUN on a storage device gets remapped while in-use by multipath,
> >it's possible that the multipath device will continue writing to this
> >new LUN, causing corruption. This is not multipath's fault (users
> >should go remapping in-use LUNs), but it's possible for multipath to
> >detect this and disable IO to the device. If disable_changed_wwids
> >is set to "yes", multipathd will detect when a LUN changes wwids when it
> >receives the uevent for this, and will disable access to the device
> >until the LUN is mapped back.
> >
> >Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> >---
> > libmultipath/config.c | 1 +
> > libmultipath/config.h | 1 +
> > libmultipath/defaults.h | 1 +
> > libmultipath/dict.c | 4 ++++
> > libmultipath/discovery.c | 15 +++++++--------
> > libmultipath/discovery.h | 1 +
> > libmultipath/structs.h | 1 +
> > multipathd/main.c | 32 ++++++++++++++++++++++++++++++++
> > 8 files changed, 48 insertions(+), 8 deletions(-)
> >
> Hmm. Not sure if the really buys us anything. By the time we process the
> uevent it might already be too late, and I/O might already have been written
> to that device.
> I do agree on the warning, though.
I have taken some pains to try to explain this short-coming to the
customer who filed this bugzilla. The good news is that in practice, the
device ususally goes down when you are remapping the LUN, and you
disable the device before it ever comes back. In my testing, I wasn't
able to actually ever write to the wrong device with
disable_changed_wwids enabled, but I do agree that you have the
potential to race here.
-Ben
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke zSeries & Storage
> hare@suse.de +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 09/10] add disable_changed_wwids option
2016-10-29 2:55 ` [PATCH 09/10] add disable_changed_wwids option Benjamin Marzinski
2016-10-30 13:54 ` Hannes Reinecke
@ 2016-11-06 0:03 ` Xose Vazquez Perez
2016-11-07 14:47 ` Benjamin Marzinski
1 sibling, 1 reply; 29+ messages in thread
From: Xose Vazquez Perez @ 2016-11-06 0:03 UTC (permalink / raw)
To: Benjamin Marzinski, Christophe Varoqui; +Cc: device-mapper development
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> If a LUN on a storage device gets remapped while in-use by multipath,
> it's possible that the multipath device will continue writing to this
> new LUN, causing corruption. This is not multipath's fault (users
> should go remapping in-use LUNs), but it's possible for multipath to
> detect this and disable IO to the device. If disable_changed_wwids
> is set to "yes", multipathd will detect when a LUN changes wwids when it
> receives the uevent for this, and will disable access to the device
> until the LUN is mapped back.
Some info is needed at multipath.conf.5 for this new keyword.
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/config.c | 1 +
> libmultipath/config.h | 1 +
> libmultipath/defaults.h | 1 +
> libmultipath/dict.c | 4 ++++
> libmultipath/discovery.c | 15 +++++++--------
> libmultipath/discovery.h | 1 +
> libmultipath/structs.h | 1 +
> multipathd/main.c | 32 ++++++++++++++++++++++++++++++++
> 8 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index bdcad80..a97b318 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -618,6 +618,7 @@ load_config (char * file)
> conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT;
> conf->deferred_remove = DEFAULT_DEFERRED_REMOVE;
> conf->skip_kpartx = DEFAULT_SKIP_KPARTX;
> + conf->disable_changed_wwids = DEFAULT_DISABLE_CHANGED_WWIDS;
>
> /*
> * preload default hwtable
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index d59a993..dbdaa44 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -144,6 +144,7 @@ struct config {
> int delayed_reconfig;
> int uev_wait_timeout;
> int skip_kpartx;
> + int disable_changed_wwids;
> unsigned int version[3];
>
> char * multipath_dir;
> diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
> index a1fee9b..a72078f 100644
> --- a/libmultipath/defaults.h
> +++ b/libmultipath/defaults.h
> @@ -36,6 +36,7 @@
> #define DEFAULT_FORCE_SYNC 0
> #define DEFAULT_PARTITION_DELIM NULL
> #define DEFAULT_SKIP_KPARTX SKIP_KPARTX_OFF
> +#define DEFAULT_DISABLE_CHANGED_WWIDS 0
>
> #define DEFAULT_CHECKINT 5
> #define MAX_CHECKINT(a) (a << 2)
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index e0a3014..61b6910 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -412,6 +412,9 @@ declare_hw_snprint(skip_kpartx, print_yes_no_undef)
> declare_mp_handler(skip_kpartx, set_yes_no_undef)
> declare_mp_snprint(skip_kpartx, print_yes_no_undef)
>
> +declare_def_handler(disable_changed_wwids, set_yes_no)
> +declare_def_snprint(disable_changed_wwids, print_yes_no)
> +
> static int
> def_config_dir_handler(struct config *conf, vector strvec)
> {
> @@ -1395,6 +1398,7 @@ init_keywords(vector keywords)
> install_keyword("retrigger_delay", &def_retrigger_delay_handler, &snprint_def_retrigger_delay);
> install_keyword("missing_uev_wait_timeout", &def_uev_wait_timeout_handler, &snprint_def_uev_wait_timeout);
> install_keyword("skip_kpartx", &def_skip_kpartx_handler, &snprint_def_skip_kpartx);
> + install_keyword("disable_changed_wwids", &def_disable_changed_wwids_handler, &snprint_def_disable_changed_wwids);
> __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/discovery.c b/libmultipath/discovery.c
> index bb3116d..756344f 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1538,13 +1538,12 @@ get_prio (struct path * pp)
> }
>
> static int
> -get_udev_uid(struct path * pp, char *uid_attribute)
> +get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
> {
> ssize_t len;
> const char *value;
>
> - value = udev_device_get_property_value(pp->udev,
> - uid_attribute);
> + value = udev_device_get_property_value(udev, uid_attribute);
> if (!value || strlen(value) == 0)
> value = getenv(uid_attribute);
> if (value && strlen(value)) {
> @@ -1625,8 +1624,8 @@ get_vpd_uid(struct path * pp)
> return get_vpd_sysfs(parent, 0x83, pp->wwid, WWID_SIZE);
> }
>
> -static int
> -get_uid (struct path * pp, int path_state)
> +int
> +get_uid (struct path * pp, int path_state, struct udev_device *udev)
> {
> char *c;
> const char *origin = "unknown";
> @@ -1639,7 +1638,7 @@ get_uid (struct path * pp, int path_state)
> put_multipath_config(conf);
> }
>
> - if (!pp->udev) {
> + if (!udev) {
> condlog(1, "%s: no udev information", pp->dev);
> return 1;
> }
> @@ -1669,7 +1668,7 @@ get_uid (struct path * pp, int path_state)
> int retrigger;
>
> if (pp->uid_attribute) {
> - len = get_udev_uid(pp, pp->uid_attribute);
> + len = get_udev_uid(pp, pp->uid_attribute, udev);
> origin = "udev";
> if (len <= 0)
> condlog(1,
> @@ -1798,7 +1797,7 @@ pathinfo (struct path *pp, struct config *conf, int mask)
> }
>
> if ((mask & DI_WWID) && !strlen(pp->wwid)) {
> - get_uid(pp, path_state);
> + get_uid(pp, path_state, pp->udev);
> if (!strlen(pp->wwid)) {
> pp->initialized = INIT_MISSING_UDEV;
> pp->tick = conf->retrigger_delay;
> diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
> index 0f5b1e6..176eac1 100644
> --- a/libmultipath/discovery.h
> +++ b/libmultipath/discovery.h
> @@ -49,6 +49,7 @@ ssize_t sysfs_get_vpd (struct udev_device * udev, int pg, unsigned char * buff,
> size_t len);
> int sysfs_get_asymmetric_access_state(struct path *pp,
> char *buff, int buflen);
> +int get_uid(struct path * pp, int path_state, struct udev_device *udev);
>
> /*
> * discovery bitmask
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 3a716d8..58508f6 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -217,6 +217,7 @@ struct path {
> int fd;
> int initialized;
> int retriggers;
> + int wwid_changed;
>
> /* configlet pointers */
> struct hwentry * hwe;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index dbb4554..bb96cca 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -958,6 +958,12 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
> {
> int ro, retval = 0;
> struct path * pp;
> + struct config *conf;
> + int disable_changed_wwids;
> +
> + conf = get_multipath_config();
> + disable_changed_wwids = conf->disable_changed_wwids;
> + put_multipath_config(conf);
>
> ro = uevent_get_disk_ro(uev);
>
> @@ -969,6 +975,25 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
> if (pp) {
> struct multipath *mpp = pp->mpp;
>
> + if (disable_changed_wwids &&
> + (strlen(pp->wwid) || pp->wwid_changed)) {
> + char wwid[WWID_SIZE];
> +
> + strcpy(wwid, pp->wwid);
> + get_uid(pp, pp->state, uev->udev);
> + if (strcmp(wwid, pp->wwid) != 0) {
> + condlog(0, "%s: path wwid changed from '%s' to '%s'. disallowing", uev->kernel, wwid, pp->wwid);
> + strcpy(pp->wwid, wwid);
> + if (!pp->wwid_changed) {
> + pp->wwid_changed = 1;
> + pp->tick = 1;
> + dm_fail_path(pp->mpp->alias, pp->dev_t);
> + }
> + goto out;
> + } else
> + pp->wwid_changed = 0;
> + }
> +
> if (pp->initialized == INIT_REQUESTED_UDEV)
> retval = uev_add_path(uev, vecs);
> else if (mpp && ro >= 0) {
> @@ -983,6 +1008,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
> }
> }
> }
> +out:
> lock_cleanup_pop(vecs->lock);
> if (!pp)
> condlog(0, "%s: spurious uevent, path not found", uev->kernel);
> @@ -1509,6 +1535,12 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> } else
> checker_clear_message(&pp->checker);
>
> + if (pp->wwid_changed) {
> + condlog(2, "%s: path wwid has changed. Refusing to use",
> + pp->dev);
> + newstate = PATH_DOWN;
> + }
> +
> if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
> condlog(2, "%s: unusable path", pp->dev);
> conf = get_multipath_config();
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 09/10] add disable_changed_wwids option
2016-11-06 0:03 ` Xose Vazquez Perez
@ 2016-11-07 14:47 ` Benjamin Marzinski
0 siblings, 0 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2016-11-07 14:47 UTC (permalink / raw)
To: Xose Vazquez Perez; +Cc: device-mapper development
On Sun, Nov 06, 2016 at 01:03:31AM +0100, Xose Vazquez Perez wrote:
> On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
>
> > If a LUN on a storage device gets remapped while in-use by multipath,
> > it's possible that the multipath device will continue writing to this
> > new LUN, causing corruption. This is not multipath's fault (users
> > should go remapping in-use LUNs), but it's possible for multipath to
> > detect this and disable IO to the device. If disable_changed_wwids
> > is set to "yes", multipathd will detect when a LUN changes wwids when it
> > receives the uevent for this, and will disable access to the device
> > until the LUN is mapped back.
>
> Some info is needed at multipath.conf.5 for this new keyword.
>
Oops! Thanks. I'll send a patch with the manpage info.
-Ben
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > libmultipath/config.c | 1 +
> > libmultipath/config.h | 1 +
> > libmultipath/defaults.h | 1 +
> > libmultipath/dict.c | 4 ++++
> > libmultipath/discovery.c | 15 +++++++--------
> > libmultipath/discovery.h | 1 +
> > libmultipath/structs.h | 1 +
> > multipathd/main.c | 32 ++++++++++++++++++++++++++++++++
> > 8 files changed, 48 insertions(+), 8 deletions(-)
> >
> > diff --git a/libmultipath/config.c b/libmultipath/config.c
> > index bdcad80..a97b318 100644
> > --- a/libmultipath/config.c
> > +++ b/libmultipath/config.c
> > @@ -618,6 +618,7 @@ load_config (char * file)
> > conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT;
> > conf->deferred_remove = DEFAULT_DEFERRED_REMOVE;
> > conf->skip_kpartx = DEFAULT_SKIP_KPARTX;
> > + conf->disable_changed_wwids = DEFAULT_DISABLE_CHANGED_WWIDS;
> >
> > /*
> > * preload default hwtable
> > diff --git a/libmultipath/config.h b/libmultipath/config.h
> > index d59a993..dbdaa44 100644
> > --- a/libmultipath/config.h
> > +++ b/libmultipath/config.h
> > @@ -144,6 +144,7 @@ struct config {
> > int delayed_reconfig;
> > int uev_wait_timeout;
> > int skip_kpartx;
> > + int disable_changed_wwids;
> > unsigned int version[3];
> >
> > char * multipath_dir;
> > diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
> > index a1fee9b..a72078f 100644
> > --- a/libmultipath/defaults.h
> > +++ b/libmultipath/defaults.h
> > @@ -36,6 +36,7 @@
> > #define DEFAULT_FORCE_SYNC 0
> > #define DEFAULT_PARTITION_DELIM NULL
> > #define DEFAULT_SKIP_KPARTX SKIP_KPARTX_OFF
> > +#define DEFAULT_DISABLE_CHANGED_WWIDS 0
> >
> > #define DEFAULT_CHECKINT 5
> > #define MAX_CHECKINT(a) (a << 2)
> > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > index e0a3014..61b6910 100644
> > --- a/libmultipath/dict.c
> > +++ b/libmultipath/dict.c
> > @@ -412,6 +412,9 @@ declare_hw_snprint(skip_kpartx, print_yes_no_undef)
> > declare_mp_handler(skip_kpartx, set_yes_no_undef)
> > declare_mp_snprint(skip_kpartx, print_yes_no_undef)
> >
> > +declare_def_handler(disable_changed_wwids, set_yes_no)
> > +declare_def_snprint(disable_changed_wwids, print_yes_no)
> > +
> > static int
> > def_config_dir_handler(struct config *conf, vector strvec)
> > {
> > @@ -1395,6 +1398,7 @@ init_keywords(vector keywords)
> > install_keyword("retrigger_delay", &def_retrigger_delay_handler, &snprint_def_retrigger_delay);
> > install_keyword("missing_uev_wait_timeout", &def_uev_wait_timeout_handler, &snprint_def_uev_wait_timeout);
> > install_keyword("skip_kpartx", &def_skip_kpartx_handler, &snprint_def_skip_kpartx);
> > + install_keyword("disable_changed_wwids", &def_disable_changed_wwids_handler, &snprint_def_disable_changed_wwids);
> > __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/discovery.c b/libmultipath/discovery.c
> > index bb3116d..756344f 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1538,13 +1538,12 @@ get_prio (struct path * pp)
> > }
> >
> > static int
> > -get_udev_uid(struct path * pp, char *uid_attribute)
> > +get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
> > {
> > ssize_t len;
> > const char *value;
> >
> > - value = udev_device_get_property_value(pp->udev,
> > - uid_attribute);
> > + value = udev_device_get_property_value(udev, uid_attribute);
> > if (!value || strlen(value) == 0)
> > value = getenv(uid_attribute);
> > if (value && strlen(value)) {
> > @@ -1625,8 +1624,8 @@ get_vpd_uid(struct path * pp)
> > return get_vpd_sysfs(parent, 0x83, pp->wwid, WWID_SIZE);
> > }
> >
> > -static int
> > -get_uid (struct path * pp, int path_state)
> > +int
> > +get_uid (struct path * pp, int path_state, struct udev_device *udev)
> > {
> > char *c;
> > const char *origin = "unknown";
> > @@ -1639,7 +1638,7 @@ get_uid (struct path * pp, int path_state)
> > put_multipath_config(conf);
> > }
> >
> > - if (!pp->udev) {
> > + if (!udev) {
> > condlog(1, "%s: no udev information", pp->dev);
> > return 1;
> > }
> > @@ -1669,7 +1668,7 @@ get_uid (struct path * pp, int path_state)
> > int retrigger;
> >
> > if (pp->uid_attribute) {
> > - len = get_udev_uid(pp, pp->uid_attribute);
> > + len = get_udev_uid(pp, pp->uid_attribute, udev);
> > origin = "udev";
> > if (len <= 0)
> > condlog(1,
> > @@ -1798,7 +1797,7 @@ pathinfo (struct path *pp, struct config *conf, int mask)
> > }
> >
> > if ((mask & DI_WWID) && !strlen(pp->wwid)) {
> > - get_uid(pp, path_state);
> > + get_uid(pp, path_state, pp->udev);
> > if (!strlen(pp->wwid)) {
> > pp->initialized = INIT_MISSING_UDEV;
> > pp->tick = conf->retrigger_delay;
> > diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
> > index 0f5b1e6..176eac1 100644
> > --- a/libmultipath/discovery.h
> > +++ b/libmultipath/discovery.h
> > @@ -49,6 +49,7 @@ ssize_t sysfs_get_vpd (struct udev_device * udev, int pg, unsigned char * buff,
> > size_t len);
> > int sysfs_get_asymmetric_access_state(struct path *pp,
> > char *buff, int buflen);
> > +int get_uid(struct path * pp, int path_state, struct udev_device *udev);
> >
> > /*
> > * discovery bitmask
> > diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> > index 3a716d8..58508f6 100644
> > --- a/libmultipath/structs.h
> > +++ b/libmultipath/structs.h
> > @@ -217,6 +217,7 @@ struct path {
> > int fd;
> > int initialized;
> > int retriggers;
> > + int wwid_changed;
> >
> > /* configlet pointers */
> > struct hwentry * hwe;
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index dbb4554..bb96cca 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -958,6 +958,12 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
> > {
> > int ro, retval = 0;
> > struct path * pp;
> > + struct config *conf;
> > + int disable_changed_wwids;
> > +
> > + conf = get_multipath_config();
> > + disable_changed_wwids = conf->disable_changed_wwids;
> > + put_multipath_config(conf);
> >
> > ro = uevent_get_disk_ro(uev);
> >
> > @@ -969,6 +975,25 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
> > if (pp) {
> > struct multipath *mpp = pp->mpp;
> >
> > + if (disable_changed_wwids &&
> > + (strlen(pp->wwid) || pp->wwid_changed)) {
> > + char wwid[WWID_SIZE];
> > +
> > + strcpy(wwid, pp->wwid);
> > + get_uid(pp, pp->state, uev->udev);
> > + if (strcmp(wwid, pp->wwid) != 0) {
> > + condlog(0, "%s: path wwid changed from '%s' to '%s'. disallowing", uev->kernel, wwid, pp->wwid);
> > + strcpy(pp->wwid, wwid);
> > + if (!pp->wwid_changed) {
> > + pp->wwid_changed = 1;
> > + pp->tick = 1;
> > + dm_fail_path(pp->mpp->alias, pp->dev_t);
> > + }
> > + goto out;
> > + } else
> > + pp->wwid_changed = 0;
> > + }
> > +
> > if (pp->initialized == INIT_REQUESTED_UDEV)
> > retval = uev_add_path(uev, vecs);
> > else if (mpp && ro >= 0) {
> > @@ -983,6 +1008,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
> > }
> > }
> > }
> > +out:
> > lock_cleanup_pop(vecs->lock);
> > if (!pp)
> > condlog(0, "%s: spurious uevent, path not found", uev->kernel);
> > @@ -1509,6 +1535,12 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> > } else
> > checker_clear_message(&pp->checker);
> >
> > + if (pp->wwid_changed) {
> > + condlog(2, "%s: path wwid has changed. Refusing to use",
> > + pp->dev);
> > + newstate = PATH_DOWN;
> > + }
> > +
> > if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
> > condlog(2, "%s: unusable path", pp->dev);
> > conf = get_multipath_config();
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 10/10] set retrigger_tries to 0 for multipath
2016-10-29 2:55 ` [PATCH 10/10] set retrigger_tries to 0 for multipath Benjamin Marzinski
2016-10-30 13:54 ` Hannes Reinecke
@ 2016-12-06 15:11 ` Xose Vazquez Perez
2016-12-06 15:22 ` Benjamin Marzinski
1 sibling, 1 reply; 29+ messages in thread
From: Xose Vazquez Perez @ 2016-12-06 15:11 UTC (permalink / raw)
To: Benjamin Marzinski, Christophe Varoqui; +Cc: device-mapper development
On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> Multipathd uses retrigger_tries to give udev more chances to to fill in
> the uid_attribute, so that the path device is correctly set up in the
> udev database. However the multipath command can't do this, so it should
> just immediately give up on udev, and try to get the wwid directly.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> multipath/main.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/multipath/main.c b/multipath/main.c
> index ee00fdb..06add30 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -521,6 +521,7 @@ main (int argc, char *argv[])
> if (!conf)
> exit(1);
> multipath_conf = conf;
> + conf->retrigger_tries = 0;
> while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BritquwW")) != EOF ) {
> switch(arg) {
> case 1: printf("optarg : %s\n",optarg);
>
I don't know how, but this patch(?) is overwriting the default value:
libmultipath/defaults.h:#define DEFAULT_RETRIGGER_TRIES 3
libmultipath/config.c: conf->retrigger_tries = DEFAULT_RETRIGGER_TRIES;
# multipath -t
defaults {
[...]
retrigger_tries 0
[...]
}
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 10/10] set retrigger_tries to 0 for multipath
2016-12-06 15:11 ` Xose Vazquez Perez
@ 2016-12-06 15:22 ` Benjamin Marzinski
2017-04-25 14:07 ` Xose Vazquez Perez
0 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2016-12-06 15:22 UTC (permalink / raw)
To: Xose Vazquez Perez; +Cc: device-mapper development
On Tue, Dec 06, 2016 at 04:11:42PM +0100, Xose Vazquez Perez wrote:
> On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
>
> > Multipathd uses retrigger_tries to give udev more chances to to fill in
> > the uid_attribute, so that the path device is correctly set up in the
> > udev database. However the multipath command can't do this, so it should
> > just immediately give up on udev, and try to get the wwid directly.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > multipath/main.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/multipath/main.c b/multipath/main.c
> > index ee00fdb..06add30 100644
> > --- a/multipath/main.c
> > +++ b/multipath/main.c
> > @@ -521,6 +521,7 @@ main (int argc, char *argv[])
> > if (!conf)
> > exit(1);
> > multipath_conf = conf;
> > + conf->retrigger_tries = 0;
> > while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BritquwW")) != EOF ) {
> > switch(arg) {
> > case 1: printf("optarg : %s\n",optarg);
> >
>
> I don't know how, but this patch(?) is overwriting the default value:
That's because multipath and mutipathd need to do different things.
However, I can change the patch so that 'multipath -t' still reports the
configured value.
-Ben
>
> libmultipath/defaults.h:#define DEFAULT_RETRIGGER_TRIES 3
> libmultipath/config.c: conf->retrigger_tries = DEFAULT_RETRIGGER_TRIES;
>
> # multipath -t
> defaults {
> [...]
> retrigger_tries 0
> [...]
> }
> [...]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 09/10] add disable_changed_wwids option
2016-11-04 15:32 ` Benjamin Marzinski
@ 2017-02-28 12:27 ` Zhangguanghui
0 siblings, 0 replies; 29+ messages in thread
From: Zhangguanghui @ 2017-02-28 12:27 UTC (permalink / raw)
To: dm-devel-bounces, Hannes Reinecke; +Cc: dm-devel
[-- Attachment #1.1: Type: text/plain, Size: 5186 bytes --]
Hi,
everyone
In my testing in the use of 3Par storage device, when a LUN changes LUN number
multipathd can not receive the uevent and can not disable access to the device.
but for HP 6300 storage device, it can receive the uevent and disable access to the device.
So I think 3Par storage device don't support the case.
Now I don't have a right way to deal with the case. Thanks for your advise.
root@ubuntu98:~# udevadm monitor
monitor will print the received events for:
UDEV - the event which udev sends out after rule processing
KERNEL - the kernel uevent
KERNEL[68031.677399] change /devices/virtual/block/dm-1 (block)
UDEV [68031.711148] change /devices/virtual/block/dm-1 (block)
KERNEL[68032.678563] change /devices/virtual/block/dm-1 (block)
UDEV [68032.705265] change /devices/virtual/block/dm-1 (block)
KERNEL[68033.681120] change /devices/virtual/block/dm-1 (block)
UDEV [68033.709744] change /devices/virtual/block/dm-1 (block)
KERNEL[68035.682421] change /devices/virtual/block/dm-1 (block)
KERNEL[68051.693362] change /devices/virtual/block/dm-2 (block)
UDEV [68051.722144] change /devices/virtual/block/dm-2 (block)
KERNEL[68052.694393] change /devices/virtual/block/dm-2 (block)
KERNEL[68052.694761] change /devices/virtual/block/dm-2 (block)
UDEV [68052.721103] change /devices/virtual/block/dm-2 (block)
root@ubuntu98:~# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 16.04 LTS
Release: 16.04
Codename: xenial
________________________________
All the best wishes for you.
zhangguanghui
From: dm-devel-bounces@redhat.com<mailto:dm-devel-bounces@redhat.com>
Date: 2016-11-04 23:32
To: Hannes Reinecke<mailto:hare@suse.de>
CC: dm-devel@redhat.com<mailto:dm-devel@redhat.com>
Subject: Re: [dm-devel] [PATCH 09/10] add disable_changed_wwids option
On Sun, Oct 30, 2016 at 02:54:14PM +0100, Hannes Reinecke wrote:
> On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
> >If a LUN on a storage device gets remapped while in-use by multipath,
> >it's possible that the multipath device will continue writing to this
> >new LUN, causing corruption. This is not multipath's fault (users
> >should go remapping in-use LUNs), but it's possible for multipath to
> >detect this and disable IO to the device. If disable_changed_wwids
> >is set to "yes", multipathd will detect when a LUN changes wwids when it
> >receives the uevent for this, and will disable access to the device
> >until the LUN is mapped back.
> >
> >Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> >---
> > libmultipath/config.c | 1 +
> > libmultipath/config.h | 1 +
> > libmultipath/defaults.h | 1 +
> > libmultipath/dict.c | 4 ++++
> > libmultipath/discovery.c | 15 +++++++--------
> > libmultipath/discovery.h | 1 +
> > libmultipath/structs.h | 1 +
> > multipathd/main.c | 32 ++++++++++++++++++++++++++++++++
> > 8 files changed, 48 insertions(+), 8 deletions(-)
> >
> Hmm. Not sure if the really buys us anything. By the time we process the
> uevent it might already be too late, and I/O might already have been written
> to that device.
> I do agree on the warning, though.
I have taken some pains to try to explain this short-coming to the
customer who filed this bugzilla. The good news is that in practice, the
device ususally goes down when you are remapping the LUN, and you
disable the device before it ever comes back. In my testing, I wasn't
able to actually ever write to the wrong device with
disable_changed_wwids enabled, but I do agree that you have the
potential to race here.
-Ben
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke zSeries & Storage
> hare@suse.de +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
>
> --
> 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
-------------------------------------------------------------------------------------------------------------------------------------
本邮件及其附件含有杭州华三通信技术有限公司的保密信息，仅限于发送给上面地址中列出
的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、
或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本
邮件！
This e-mail and its attachments contain confidential information from H3C, which is
intended only for the person or entity whose address is listed above. Any use of the
information contained herein in any way (including, but not limited to, total or partial
disclosure, reproduction, or dissemination) by persons other than the intended
recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender
by phone or email immediately and delete it!
[-- Attachment #1.2: Type: text/html, Size: 10702 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 10/10] set retrigger_tries to 0 for multipath
2016-12-06 15:22 ` Benjamin Marzinski
@ 2017-04-25 14:07 ` Xose Vazquez Perez
0 siblings, 0 replies; 29+ messages in thread
From: Xose Vazquez Perez @ 2017-04-25 14:07 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
On 12/06/2016 04:22 PM, Benjamin Marzinski wrote:
> On Tue, Dec 06, 2016 at 04:11:42PM +0100, Xose Vazquez Perez wrote:
>> On 10/29/2016 04:55 AM, Benjamin Marzinski wrote:
>>
>>> Multipathd uses retrigger_tries to give udev more chances to to fill in
>>> the uid_attribute, so that the path device is correctly set up in the
>>> udev database. However the multipath command can't do this, so it should
>>> just immediately give up on udev, and try to get the wwid directly.
>>>
>>> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
>>> ---
>>> multipath/main.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/multipath/main.c b/multipath/main.c
>>> index ee00fdb..06add30 100644
>>> --- a/multipath/main.c
>>> +++ b/multipath/main.c
>>> @@ -521,6 +521,7 @@ main (int argc, char *argv[])
>>> if (!conf)
>>> exit(1);
>>> multipath_conf = conf;
>>> + conf->retrigger_tries = 0;
>>> while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BritquwW")) != EOF ) {
>>> switch(arg) {
>>> case 1: printf("optarg : %s\n",optarg);
>>>
>>
>> I don't know how, but this patch(?) is overwriting the default value:
>
> That's because multipath and mutipathd need to do different things.
> However, I can change the patch so that 'multipath -t' still reports the
> configured value.
Could you do a patch to fix it?
Thank you.
>>
>> libmultipath/defaults.h:#define DEFAULT_RETRIGGER_TRIES 3
>> libmultipath/config.c: conf->retrigger_tries = DEFAULT_RETRIGGER_TRIES;
>>
>> # multipath -t
>> defaults {
>> [...]
>> retrigger_tries 0
>> [...]
>> }
>> [...]
>
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2017-04-25 14:07 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-29 2:55 [PATCH 00/10] misc. multipath patches Benjamin Marzinski
2016-10-29 2:55 ` [PATCH 01/10] libmultipath: add skip_kpartx option Benjamin Marzinski
2016-10-30 13:42 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 02/10] kpartx.rules: respect skip_kpartx flag Benjamin Marzinski
2016-10-30 13:42 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 03/10] do not allow in-use path to change wwid Benjamin Marzinski
2016-10-30 13:45 ` Hannes Reinecke
2016-10-31 14:30 ` Benjamin Marzinski
2016-10-29 2:55 ` [PATCH 04/10] multipathd: add "map failures" format wildcard Benjamin Marzinski
2016-10-30 13:45 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 05/10] mpath: don't wait for udev if all paths are down Benjamin Marzinski
2016-10-30 13:46 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 06/10] multipath: set cookie before using it Benjamin Marzinski
2016-10-30 13:46 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 07/10] recover from errors in multipathd startup Benjamin Marzinski
2016-10-30 13:47 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 08/10] fix INIT_REQUESTED_UDEV code Benjamin Marzinski
2016-10-30 13:48 ` Hannes Reinecke
2016-10-29 2:55 ` [PATCH 09/10] add disable_changed_wwids option Benjamin Marzinski
2016-10-30 13:54 ` Hannes Reinecke
2016-11-04 15:32 ` Benjamin Marzinski
2017-02-28 12:27 ` Zhangguanghui
2016-11-06 0:03 ` Xose Vazquez Perez
2016-11-07 14:47 ` Benjamin Marzinski
2016-10-29 2:55 ` [PATCH 10/10] set retrigger_tries to 0 for multipath Benjamin Marzinski
2016-10-30 13:54 ` Hannes Reinecke
2016-12-06 15:11 ` Xose Vazquez Perez
2016-12-06 15:22 ` Benjamin Marzinski
2017-04-25 14:07 ` Xose Vazquez Perez
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.