Re: [PATCH v3 10/11] libmultipath: don't try to set hwhandler if it is retained

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
> ---
>  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
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@xxxxxxx			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux