| Message ID | 20170621150630.25773-11-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On 06/21/2017 05:06 PM, Martin Wilck wrote: > Setting a device handler only works if retain_attached_hw_handler > is 'no', or if the kernel didn't auto-assign a handler. If this > is not the case, don't even attempt to set a different handler. > > This requires reading the sysfs "dh_state" path attribute. > For internal consistency, this attribute must be updated after domap(). > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/configure.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++- > libmultipath/discovery.c | 4 ++++ > libmultipath/propsel.c | 15 ++++++++++++++ > libmultipath/structs.h | 2 ++ > 4 files changed, 72 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > index 74b6f52a..55dbb261 100644 > --- a/libmultipath/configure.c > +++ b/libmultipath/configure.c > @@ -275,15 +275,21 @@ int setup_map(struct multipath *mpp, char *params, int params_size) > > /* > * properties selectors > + * > + * Ordering matters for some properties: > + * - features after no_path_retry and retain_hwhandler > + * - hwhandler after retain_hwhandler > + * No guarantee that this list is complete, check code in > + * propsel.c if in doubt. > */ > conf = get_multipath_config(); > select_pgfailback(conf, mpp); > select_pgpolicy(conf, mpp); > select_selector(conf, mpp); > - select_hwhandler(conf, mpp); > select_no_path_retry(conf, mpp); > select_retain_hwhandler(conf, mpp); > select_features(conf, mpp); > + select_hwhandler(conf, mpp); > select_rr_weight(conf, mpp); > select_minio(conf, mpp); > select_mode(conf, mpp); > @@ -706,6 +712,48 @@ fail: > return 1; > } > > +static void > +check_dh_state_changed(struct multipath *mp) > +{ > + struct config *conf; > + struct path newp, *pp; > + struct pathgroup *pg; > + int i, j; > + > + conf = get_multipath_config(); > + > + vector_foreach_slot (mp->pg, pg, j) { > + vector_foreach_slot (pg->paths, pp, i) { > + if (!pp->udev || !strlen(pp->dh_state) || > + (conf->retain_hwhandler == RETAIN_HWHANDLER_ON && > + strcmp(pp->dh_state, "detached"))) > + continue; > + > + memset(&newp, 0, sizeof(newp)); > + memcpy(newp.dev, pp->dev, sizeof(newp.dev)); > + newp.udev = udev_device_ref(pp->udev); > + > + if (pathinfo(&newp, conf, DI_SYSFS) == PATHINFO_OK) { > + if (strncmp(newp.dh_state, pp->dh_state, > + SCSI_DH_SIZE)) { > + condlog(3, "%s: dh_state changed from %s to %s", > + pp->dev, > + pp->dh_state, > + newp.dh_state); > + memcpy(pp->dh_state, newp.dh_state, > + SCSI_DH_SIZE); > + } > + } else > + condlog(1, "%s: failed to update dh_state", > + pp->dev); > + > + udev_device_unref(newp.udev); > + } > + } > + > + put_multipath_config(conf); > +} > + > /* > * Return value: > */ I would avoid using a temporary udev device here; either save the dh_state value and call pathinfo() on the _actual_ device or read dh_state directly from sysfs without an intermediate udev device. > @@ -828,6 +876,8 @@ int domap(struct multipath *mpp, char *params, int is_daemon) > } > } > dm_setgeometry(mpp); > + > + check_dh_state_changed(mpp); > return DOMAP_OK; > } > return DOMAP_FAIL; > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index 663c8eaa..8c0c6a9f 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -1188,6 +1188,10 @@ scsi_sysfs_pathinfo (struct path * pp, vector hwtable) > condlog(3, "%s: tgt_node_name = %s", > pp->dev, pp->tgt_node_name); > > + if (sysfs_attr_get_value(parent, "dh_state", > + pp->dh_state, sizeof(pp->dh_state)) >= 0) > + condlog(3, "%s: dh_state = %s", pp->dev, pp->dh_state); > + > return 0; > } > > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c > index d609394e..d1b3d416 100644 > --- a/libmultipath/propsel.c > +++ b/libmultipath/propsel.c > @@ -345,7 +345,22 @@ out: > int select_hwhandler(struct config *conf, struct multipath *mp) > { > char *origin; > + struct path *pp; > + char handler[SCSI_DH_SIZE+2]; > + int i; > > + if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) { > + vector_foreach_slot(mp->paths, pp, i) { > + if (strlen(pp->dh_state) && > + strcmp(pp->dh_state, "detached")) { > + snprintf(handler, sizeof(handler), > + "1 %s", pp->dh_state); > + mp->hwhandler = handler; > + origin = "(setting: retained by kernel driver)"; > + goto out; > + } > + } > + } > mp_set_hwe(hwhandler); > mp_set_conf(hwhandler); > mp_set_default(hwhandler, DEFAULT_HWHANDLER); > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > index 8ea984d9..4203e2b0 100644 > --- a/libmultipath/structs.h > +++ b/libmultipath/structs.h > @@ -22,6 +22,7 @@ > #define SCSI_PRODUCT_SIZE 17 > #define SCSI_REV_SIZE 5 > #define SCSI_STATE_SIZE 19 > +#define SCSI_DH_SIZE 9 /* must hold "detached" */ > > #define NO_PATH_RETRY_UNDEF 0 > #define NO_PATH_RETRY_FAIL -1 > @@ -205,6 +206,7 @@ struct path { > char rev[SCSI_REV_SIZE]; > char serial[SERIAL_SIZE]; > char tgt_node_name[NODE_NAME_SIZE]; > + char dh_state[SCSI_DH_SIZE]; > unsigned long long size; > unsigned int checkint; > unsigned int tick; > Hmm. Not sure if I fully agree with this. I do see the need to read 'dh_state' from pathinfo(), just to figure out if an hardware handler is already loaded. But once select_hwhandler is done it's quite pointless to update the dh_state; what exactly _would_ be the error action in this case? Plus the code detects the failure, but then doesn't do anything with it... So, please, if you insist on checking dh_state please implement correct error action here, like updating the 'hwhandler' value to that found in dh_state or disabling the hardware handler if it's found to be detached. Cheers, Hannes
On Thu, 2017-06-22 at 08:21 +0200, Hannes Reinecke wrote: > On 06/21/2017 05:06 PM, Martin Wilck wrote: > > Setting a device handler only works if retain_attached_hw_handler > > is 'no', or if the kernel didn't auto-assign a handler. If this > > is not the case, don't even attempt to set a different handler. > > > > This requires reading the sysfs "dh_state" path attribute. > > For internal consistency, this attribute must be updated after > > domap(). > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > --- > > libmultipath/configure.c | 52 > > +++++++++++++++++++++++++++++++++++++++++++++++- > > libmultipath/discovery.c | 4 ++++ > > libmultipath/propsel.c | 15 ++++++++++++++ > > libmultipath/structs.h | 2 ++ > > 4 files changed, 72 insertions(+), 1 deletion(-) > > > > [...] > > Hmm. > > Not sure if I fully agree with this. > I do see the need to read 'dh_state' from pathinfo(), just to figure > out > if an hardware handler is already loaded. > > But once select_hwhandler is done it's quite pointless to update the > dh_state; what exactly _would_ be the error action in this case? > Plus the code detects the failure, but then doesn't do anything with > it... My concern was that multipathd might carry along wrong state and possibly print it in log messages, irritating users. It's true, there is no reasonable error action, and it's quite a lot of code just for this minor purpose. > So, please, if you insist on checking dh_state please implement > correct > error action here, like updating the 'hwhandler' value to that found > in > dh_state or disabling the hardware handler if it's found to be > detached. If it's fine with you and other reviewers, I'll happily remove that part of the patch, and just keep the part in select_hwhandler(). If we want proper error handling, we'd need to check that the handler of the loaded map as well as the handlers of all paths are set to the handler configured in multipathd. Unless I'm mistaken, it isn't guaranteed that all paths will be using the same handler after a map is set up. Besides re-reading the dh_state of all paths, this check would also require re-reading and disassembling the map, and no reasonable error action is to be seen other then updating the internal state, lots of code for almost nothing. Cheers, Martin
diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 74b6f52a..55dbb261 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -275,15 +275,21 @@ int setup_map(struct multipath *mpp, char *params, int params_size) /* * properties selectors + * + * Ordering matters for some properties: + * - features after no_path_retry and retain_hwhandler + * - hwhandler after retain_hwhandler + * No guarantee that this list is complete, check code in + * propsel.c if in doubt. */ conf = get_multipath_config(); select_pgfailback(conf, mpp); select_pgpolicy(conf, mpp); select_selector(conf, mpp); - select_hwhandler(conf, mpp); select_no_path_retry(conf, mpp); select_retain_hwhandler(conf, mpp); select_features(conf, mpp); + select_hwhandler(conf, mpp); select_rr_weight(conf, mpp); select_minio(conf, mpp); select_mode(conf, mpp); @@ -706,6 +712,48 @@ fail: return 1; } +static void +check_dh_state_changed(struct multipath *mp) +{ + struct config *conf; + struct path newp, *pp; + struct pathgroup *pg; + int i, j; + + conf = get_multipath_config(); + + vector_foreach_slot (mp->pg, pg, j) { + vector_foreach_slot (pg->paths, pp, i) { + if (!pp->udev || !strlen(pp->dh_state) || + (conf->retain_hwhandler == RETAIN_HWHANDLER_ON && + strcmp(pp->dh_state, "detached"))) + continue; + + memset(&newp, 0, sizeof(newp)); + memcpy(newp.dev, pp->dev, sizeof(newp.dev)); + newp.udev = udev_device_ref(pp->udev); + + if (pathinfo(&newp, conf, DI_SYSFS) == PATHINFO_OK) { + if (strncmp(newp.dh_state, pp->dh_state, + SCSI_DH_SIZE)) { + condlog(3, "%s: dh_state changed from %s to %s", + pp->dev, + pp->dh_state, + newp.dh_state); + memcpy(pp->dh_state, newp.dh_state, + SCSI_DH_SIZE); + } + } else + condlog(1, "%s: failed to update dh_state", + pp->dev); + + udev_device_unref(newp.udev); + } + } + + put_multipath_config(conf); +} + /* * Return value: */ @@ -828,6 +876,8 @@ int domap(struct multipath *mpp, char *params, int is_daemon) } } dm_setgeometry(mpp); + + check_dh_state_changed(mpp); return DOMAP_OK; } return DOMAP_FAIL; diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index 663c8eaa..8c0c6a9f 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -1188,6 +1188,10 @@ scsi_sysfs_pathinfo (struct path * pp, vector hwtable) condlog(3, "%s: tgt_node_name = %s", pp->dev, pp->tgt_node_name); + if (sysfs_attr_get_value(parent, "dh_state", + pp->dh_state, sizeof(pp->dh_state)) >= 0) + condlog(3, "%s: dh_state = %s", pp->dev, pp->dh_state); + return 0; } diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c index d609394e..d1b3d416 100644 --- a/libmultipath/propsel.c +++ b/libmultipath/propsel.c @@ -345,7 +345,22 @@ out: int select_hwhandler(struct config *conf, struct multipath *mp) { char *origin; + struct path *pp; + char handler[SCSI_DH_SIZE+2]; + int i; + if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) { + vector_foreach_slot(mp->paths, pp, i) { + if (strlen(pp->dh_state) && + strcmp(pp->dh_state, "detached")) { + snprintf(handler, sizeof(handler), + "1 %s", pp->dh_state); + mp->hwhandler = handler; + origin = "(setting: retained by kernel driver)"; + goto out; + } + } + } mp_set_hwe(hwhandler); mp_set_conf(hwhandler); mp_set_default(hwhandler, DEFAULT_HWHANDLER); diff --git a/libmultipath/structs.h b/libmultipath/structs.h index 8ea984d9..4203e2b0 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -22,6 +22,7 @@ #define SCSI_PRODUCT_SIZE 17 #define SCSI_REV_SIZE 5 #define SCSI_STATE_SIZE 19 +#define SCSI_DH_SIZE 9 /* must hold "detached" */ #define NO_PATH_RETRY_UNDEF 0 #define NO_PATH_RETRY_FAIL -1 @@ -205,6 +206,7 @@ struct path { char rev[SCSI_REV_SIZE]; char serial[SERIAL_SIZE]; char tgt_node_name[NODE_NAME_SIZE]; + char dh_state[SCSI_DH_SIZE]; unsigned long long size; unsigned int checkint; unsigned int tick;
Setting a device handler only works if retain_attached_hw_handler is 'no', or if the kernel didn't auto-assign a handler. If this is not the case, don't even attempt to set a different handler. This requires reading the sysfs "dh_state" path attribute. For internal consistency, this attribute must be updated after domap(). Signed-off-by: Martin Wilck <mwilck@suse.com> --- libmultipath/configure.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++- libmultipath/discovery.c | 4 ++++ libmultipath/propsel.c | 15 ++++++++++++++ libmultipath/structs.h | 2 ++ 4 files changed, 72 insertions(+), 1 deletion(-)