| Message ID | 20180319150155.5363-11-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On Mon, Mar 19, 2018 at 04:01:45PM +0100, Martin Wilck wrote: > dm_addmap_create() is where we actually try to set up a new > multipath map. Depending on the result, mark the wwid as > failed (or not), and re-trigger an uevent if necessary. > If a path changes from multipath to non-multipath, use an "add" > event to make sure LVM2 rules pick it up. Increase log level > of this event to 3. I'm not sure of a specific problem with this patch, but I am a little leery of sending out our own "add" events. Unless I am mistaken there is usually there is only one add event per device, and there is extra work done on add events that we might not want to do twice. I wonder if it would be better to make other udev rules be able to respond to change events from ex-multipath paths. Have you looked into the implications of sending out these add events? -Ben > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/configure.c | 37 ++++++++++++++++++++++++++++--------- > libmultipath/configure.h | 2 +- > libmultipath/devmapper.c | 8 +++++++- > libmultipath/structs.h | 1 + > multipathd/main.c | 2 +- > 5 files changed, 38 insertions(+), 12 deletions(-) > > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > index 838d145a5aa2..88e6687849f8 100644 > --- a/libmultipath/configure.c > +++ b/libmultipath/configure.c > @@ -443,11 +443,18 @@ trigger_udev_change(const struct multipath *mpp) > } > > void > -trigger_paths_udev_change(const struct multipath *mpp) > +trigger_paths_udev_change(struct multipath *mpp, bool is_mpath) > { > struct pathgroup * pgp; > struct path * pp; > int i, j; > + /* > + * If a path changes from multipath to non-multipath, we must > + * synthesize an artificial "add" event, otherwise the LVM2 rules > + * (69-lvm2-lvmetad.rules) won't pick it up. Otherwise, we'd just > + * irritate ourselves with an "add", so use "change". > + */ > + const char *action = is_mpath ? "change" : "add"; > > if (!mpp || !mpp->pg) > return; > @@ -466,14 +473,21 @@ trigger_paths_udev_change(const struct multipath *mpp) > */ > env = udev_device_get_property_value( > pp->udev, "DM_MULTIPATH_DEVICE_PATH"); > - if (env != NULL && !strcmp(env, "1")) > - continue; > > - condlog(4, "triggering change uevent for %s", pp->dev); > - sysfs_attr_set_value(pp->udev, "uevent", "change", > - strlen("change")); > + if (is_mpath && env != NULL && !strcmp(env, "1")) > + continue; > + else if (!is_mpath && > + (env == NULL || !strcmp(env, "0"))) > + continue; > + > + condlog(3, "triggering %s uevent for %s (is %smultipath member)", > + action, pp->dev, is_mpath ? "" : "no "); > + sysfs_attr_set_value(pp->udev, "uevent", > + action, strlen(action)); > } > } > + > + mpp->needs_paths_uevent = 0; > } > > static int > @@ -870,8 +884,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon) > * succeeded > */ > mpp->force_udev_reload = 0; > - if (mpp->action == ACT_CREATE && remember_wwid(mpp->wwid) == 1) > - trigger_paths_udev_change(mpp); > + if (mpp->action == ACT_CREATE && > + (remember_wwid(mpp->wwid) == 1 || > + mpp->needs_paths_uevent)) > + trigger_paths_udev_change(mpp, true); > if (!is_daemon) { > /* multipath client mode */ > dm_switchgroup(mpp->alias, mpp->bestpg); > @@ -896,7 +912,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon) > } > dm_setgeometry(mpp); > return DOMAP_OK; > - } > + } else if (r == DOMAP_FAIL && mpp->action == ACT_CREATE && > + mpp->needs_paths_uevent) > + trigger_paths_udev_change(mpp, false); > + > return DOMAP_FAIL; > } > > diff --git a/libmultipath/configure.h b/libmultipath/configure.h > index 545cbc209793..8b56d33a1d0b 100644 > --- a/libmultipath/configure.h > +++ b/libmultipath/configure.h > @@ -37,4 +37,4 @@ int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type, > vector pathvec, char **wwid); > int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon); > struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type); > -void trigger_paths_udev_change(const struct multipath *mpp); > +void trigger_paths_udev_change(struct multipath *mpp, bool is_mpath); > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > index 9bafbc6a239a..bd595f4fa40b 100644 > --- a/libmultipath/devmapper.c > +++ b/libmultipath/devmapper.c > @@ -22,6 +22,7 @@ > #include "devmapper.h" > #include "sysfs.h" > #include "config.h" > +#include "wwids.h" > > #include "log_pthread.h" > #include <sys/types.h> > @@ -411,8 +412,11 @@ int dm_addmap_create (struct multipath *mpp, char * params) > int err; > > if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro, > - udev_flags)) > + udev_flags)) { > + if (unmark_failed_wwid(mpp->wwid) == 1) > + mpp->needs_paths_uevent = 1; > return 1; > + } > /* > * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD. > * Failing the second part leaves an empty map. Clean it up. > @@ -428,6 +432,8 @@ int dm_addmap_create (struct multipath *mpp, char * params) > break; > } > } > + if (mark_failed_wwid(mpp->wwid) == 1) > + mpp->needs_paths_uevent = 1; > return 0; > } > > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > index d6482f84f0e6..32f4b2d0696c 100644 > --- a/libmultipath/structs.h > +++ b/libmultipath/structs.h > @@ -322,6 +322,7 @@ struct multipath { > int max_sectors_kb; > int force_readonly; > int force_udev_reload; > + int needs_paths_uevent; > int ghost_delay; > int ghost_delay_tick; > unsigned int dev_loss; > diff --git a/multipathd/main.c b/multipathd/main.c > index bfbe5f66b324..ea8c413f28c6 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2310,7 +2310,7 @@ configure (struct vectors * vecs) > sync_maps_state(mpvec); > vector_foreach_slot(mpvec, mpp, i){ > if (remember_wwid(mpp->wwid) == 1) > - trigger_paths_udev_change(mpp); > + trigger_paths_udev_change(mpp, true); > update_map_pr(mpp); > } > > -- > 2.16.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 2018-03-26 at 12:52 -0500, Benjamin Marzinski wrote: > On Mon, Mar 19, 2018 at 04:01:45PM +0100, Martin Wilck wrote: > > dm_addmap_create() is where we actually try to set up a new > > multipath map. Depending on the result, mark the wwid as > > failed (or not), and re-trigger an uevent if necessary. > > If a path changes from multipath to non-multipath, use an "add" > > event to make sure LVM2 rules pick it up. Increase log level > > of this event to 3. > > I'm not sure of a specific problem with this patch, but I am a little > leery of sending out our own "add" events. Unless I am mistaken there > is > usually there is only one add event per device, and there is extra > work > done on add events that we might not want to do twice. I wonder if it > would be better to make other udev rules be able to respond to change > events from ex-multipath paths. Have you looked into the > implications > of sending out these add events? Yes. I reviewed the udev rules on a few current systems. There are only very few rules that act on "add" only. For those devices that matter here (block devices, non-dm) I see only the following two: 60-block.rules: a rule that enables parameters/events_dfl_poll_msecs. => not critical, we just disable/enable the polling once more. 69-dm-lvm-metad.rules:ACTION!="add", GOTO="lvm_end" => this is the rule that forced me to synthesize an "add" event. So indeed, I think that at least for the current state of affairs sending an "add" event is safe. Wrt "make other udev rules be able to respond to change events" - LVM2 commit 756bcab explains in detail why LVM2 switched to scanning on "add" events only. This code is >5 years old. It might be possible to distinguish different kinds of "change" events for lvmetad (e.g. remembering "DM_MULTIPATH_DEVICE_PATH" and re-scanning when it has changed from "1" to "0") but to be honest, I'd rather not want to depend on such a change being merged in lvm2. I've submitted a minor, IMO rather un-controversial fix for 69-dm-lvmetad.rules 3 monts ago ("LVM2: fix lvmetad udev rules for CHANGE events") and sent out multiple reminders, without receiving a reaction. Moreover, we're sending out these events for devices which have DM_MULTIPATH_DEVICE_PATH=1 and SYSTEMD_READY=0 set. For everything above the multipath layer, it's actually as if the device had just been "add"ed. I think this will either work with synthesized "add" events, or not at all. Please correct me if I've overlooked something. Martin
diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 838d145a5aa2..88e6687849f8 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -443,11 +443,18 @@ trigger_udev_change(const struct multipath *mpp) } void -trigger_paths_udev_change(const struct multipath *mpp) +trigger_paths_udev_change(struct multipath *mpp, bool is_mpath) { struct pathgroup * pgp; struct path * pp; int i, j; + /* + * If a path changes from multipath to non-multipath, we must + * synthesize an artificial "add" event, otherwise the LVM2 rules + * (69-lvm2-lvmetad.rules) won't pick it up. Otherwise, we'd just + * irritate ourselves with an "add", so use "change". + */ + const char *action = is_mpath ? "change" : "add"; if (!mpp || !mpp->pg) return; @@ -466,14 +473,21 @@ trigger_paths_udev_change(const struct multipath *mpp) */ env = udev_device_get_property_value( pp->udev, "DM_MULTIPATH_DEVICE_PATH"); - if (env != NULL && !strcmp(env, "1")) - continue; - condlog(4, "triggering change uevent for %s", pp->dev); - sysfs_attr_set_value(pp->udev, "uevent", "change", - strlen("change")); + if (is_mpath && env != NULL && !strcmp(env, "1")) + continue; + else if (!is_mpath && + (env == NULL || !strcmp(env, "0"))) + continue; + + condlog(3, "triggering %s uevent for %s (is %smultipath member)", + action, pp->dev, is_mpath ? "" : "no "); + sysfs_attr_set_value(pp->udev, "uevent", + action, strlen(action)); } } + + mpp->needs_paths_uevent = 0; } static int @@ -870,8 +884,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon) * succeeded */ mpp->force_udev_reload = 0; - if (mpp->action == ACT_CREATE && remember_wwid(mpp->wwid) == 1) - trigger_paths_udev_change(mpp); + if (mpp->action == ACT_CREATE && + (remember_wwid(mpp->wwid) == 1 || + mpp->needs_paths_uevent)) + trigger_paths_udev_change(mpp, true); if (!is_daemon) { /* multipath client mode */ dm_switchgroup(mpp->alias, mpp->bestpg); @@ -896,7 +912,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon) } dm_setgeometry(mpp); return DOMAP_OK; - } + } else if (r == DOMAP_FAIL && mpp->action == ACT_CREATE && + mpp->needs_paths_uevent) + trigger_paths_udev_change(mpp, false); + return DOMAP_FAIL; } diff --git a/libmultipath/configure.h b/libmultipath/configure.h index 545cbc209793..8b56d33a1d0b 100644 --- a/libmultipath/configure.h +++ b/libmultipath/configure.h @@ -37,4 +37,4 @@ int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type, vector pathvec, char **wwid); int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon); struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type); -void trigger_paths_udev_change(const struct multipath *mpp); +void trigger_paths_udev_change(struct multipath *mpp, bool is_mpath); diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index 9bafbc6a239a..bd595f4fa40b 100644 --- a/libmultipath/devmapper.c +++ b/libmultipath/devmapper.c @@ -22,6 +22,7 @@ #include "devmapper.h" #include "sysfs.h" #include "config.h" +#include "wwids.h" #include "log_pthread.h" #include <sys/types.h> @@ -411,8 +412,11 @@ int dm_addmap_create (struct multipath *mpp, char * params) int err; if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro, - udev_flags)) + udev_flags)) { + if (unmark_failed_wwid(mpp->wwid) == 1) + mpp->needs_paths_uevent = 1; return 1; + } /* * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD. * Failing the second part leaves an empty map. Clean it up. @@ -428,6 +432,8 @@ int dm_addmap_create (struct multipath *mpp, char * params) break; } } + if (mark_failed_wwid(mpp->wwid) == 1) + mpp->needs_paths_uevent = 1; return 0; } diff --git a/libmultipath/structs.h b/libmultipath/structs.h index d6482f84f0e6..32f4b2d0696c 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -322,6 +322,7 @@ struct multipath { int max_sectors_kb; int force_readonly; int force_udev_reload; + int needs_paths_uevent; int ghost_delay; int ghost_delay_tick; unsigned int dev_loss; diff --git a/multipathd/main.c b/multipathd/main.c index bfbe5f66b324..ea8c413f28c6 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2310,7 +2310,7 @@ configure (struct vectors * vecs) sync_maps_state(mpvec); vector_foreach_slot(mpvec, mpp, i){ if (remember_wwid(mpp->wwid) == 1) - trigger_paths_udev_change(mpp); + trigger_paths_udev_change(mpp, true); update_map_pr(mpp); }
dm_addmap_create() is where we actually try to set up a new multipath map. Depending on the result, mark the wwid as failed (or not), and re-trigger an uevent if necessary. If a path changes from multipath to non-multipath, use an "add" event to make sure LVM2 rules pick it up. Increase log level of this event to 3. Signed-off-by: Martin Wilck <mwilck@suse.com> --- libmultipath/configure.c | 37 ++++++++++++++++++++++++++++--------- libmultipath/configure.h | 2 +- libmultipath/devmapper.c | 8 +++++++- libmultipath/structs.h | 1 + multipathd/main.c | 2 +- 5 files changed, 38 insertions(+), 12 deletions(-)