[dm-devel] [PATCH] libmultipath: Allow discovery of USB devices

[Martin Wilck]

On Tue, 2020-09-22 at 20:34 +0200, Frank Meinl wrote:
> This change adds a new configuration option allow_usb_devices. It is
> disabled by default, so that the behavior of existing setups is not
> changed. If enabled (via multipath.conf), USB devices will be found
> during device discovery and can be used for multipath setups.
> 
> Without this option, multipath currently ignores all USB drives,
> which
> is convenient for most users. (refer also to commit 51957eb)
> 
> However, it can be beneficial to use the multipath dm-module even if
> there is only a single path to an USB attached storage. In
> combination
> with the option 'queue_if_no_path', such a setup survives a temporary
> device disconnect without any severe consequences for the running
> applications. All I/O is queued until the device reappears.

Hm. So you want to use multipath just to enable queueing?
I wonder if that can't be achieved some other way; multipath seems to
be quite big hammer for this nail. Anyway, I don't think this would
hurt us, so I don't have good reasons to reject it.

Waiting for others' opinions.

> 
> Signed-off-by: Frank Meinl <frank.meinl at live.de>
> ---
>  libmultipath/config.h      |  1 +
>  libmultipath/dict.c        |  4 ++++
>  libmultipath/discovery.c   | 13 ++++++++++---
>  libmultipath/structs.h     |  1 +
>  multipath/multipath.conf.5 | 14 ++++++++++++++
>  5 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 2bb7153b..290aea58 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -158,6 +158,7 @@ struct config {
>  	unsigned int dev_loss;
>  	int log_checker_err;
>  	int allow_queueing;
> +	int allow_usb_devices;
>  	int find_multipaths;
>  	uid_t uid;
>  	gid_t gid;
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index feabae56..f12c2e5c 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -543,6 +543,9 @@ snprint_def_queue_without_daemon (struct config
> *conf,
>  declare_def_handler(checker_timeout, set_int)
>  declare_def_snprint(checker_timeout, print_nonzero)
>  
> +declare_def_handler(allow_usb_devices, set_yes_no)
> +declare_def_snprint(allow_usb_devices, print_yes_no)
> +
>  declare_def_handler(flush_on_last_del, set_yes_no_undef)
>  declare_def_snprint_defint(flush_on_last_del, print_yes_no_undef,
> DEFAULT_FLUSH)
>  declare_ovr_handler(flush_on_last_del, set_yes_no_undef)
> @@ -1759,6 +1762,7 @@ init_keywords(vector keywords)
>  	install_keyword("no_path_retry", &def_no_path_retry_handler,
> &snprint_def_no_path_retry);
>  	install_keyword("queue_without_daemon",
> &def_queue_without_daemon_handler,
> &snprint_def_queue_without_daemon);
>  	install_keyword("checker_timeout",
> &def_checker_timeout_handler, &snprint_def_checker_timeout);
> +	install_keyword("allow_usb_devices",
> &def_allow_usb_devices_handler, &snprint_def_allow_usb_devices);
>  	install_keyword("pg_timeout", &deprecated_handler,
> &snprint_deprecated);
>  	install_keyword("flush_on_last_del",
> &def_flush_on_last_del_handler, &snprint_def_flush_on_last_del);
>  	install_keyword("user_friendly_names",
> &def_user_friendly_names_handler, &snprint_def_user_friendly_names);
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 2f301ac4..4b615caa 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -375,11 +375,10 @@ sysfs_get_tgt_nodename(struct path *pp, char
> *node)
>  	while (tgtdev) {
>  		value = udev_device_get_subsystem(tgtdev);
>  		if (value && !strcmp(value, "usb")) {
> -			pp->sg_id.proto_id = SCSI_PROTOCOL_UNSPEC;
> +			pp->sg_id.proto_id = SCSI_PROTOCOL_UAS;

How do you know this is UAS? It could as well be usb-storage, no?
Maybe we need not differentiate the two, but asserting UAS here
might raise wrong expectations. Maybe you should just call it
SCSI_PROTOCOL_USB.

>  			tgtname = udev_device_get_sysname(tgtdev);
>  			strlcpy(node, tgtname, NODE_NAME_SIZE);
> -			condlog(3, "%s: skip USB device %s", pp->dev,
> node);
> -			return 1;
> +			return 0;
>  		}
>  		tgtdev = udev_device_get_parent(tgtdev);
>  	}
> @@ -2136,6 +2135,14 @@ int pathinfo(struct path *pp, struct config
> *conf, int mask)
>  
>  		if (rc != PATHINFO_OK)
>  			return rc;
> +
> +		if (pp->bus == SYSFS_BUS_SCSI &&
> +		    pp->sg_id.proto_id == SCSI_PROTOCOL_UAS &&
> +		    !conf->allow_usb_devices) {
> +			condlog(3, "%s: skip USB device %s", pp->dev,
> +				pp->tgt_node_name);
> +			return PATHINFO_SKIPPED;
> +		}
>  	}
>  
>  	if (mask & DI_BLACKLIST && mask & DI_SYSFS) {
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 4afd3e88..284c1e45 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -174,6 +174,7 @@ enum scsi_protocol {
>  	SCSI_PROTOCOL_SAS = 6,
>  	SCSI_PROTOCOL_ADT = 7,	/* Media Changers */
>  	SCSI_PROTOCOL_ATA = 8,
> +	SCSI_PROTOCOL_UAS = 9,  /* USB Attached SCSI */
>  	SCSI_PROTOCOL_UNSPEC = 0xf, /* No specific protocol */
>  };
>  
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 5adaced6..21b3bfb6 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -643,6 +643,20 @@ The default is: in
> \fB/sys/block/sd<x>/device/timeout\fR
>  .
>  .
>  .TP
> +.B allow_usb_devices
> +If set to
> +.I no
> +, all USB devices will be skipped during path discovery. This is
> convenient
> +for most users, as it effectively hides all attached USB disks and
> flash
> +drives from the multipath application. However, if you intend to use
> multipath
> +on USB attached devices, set this to \fIyes\fR.
> +.RS
> +.TP
> +The default is: \fBno\fR

All factual information in the middle sentence ("This is convenient ...
application") is already present elsewhere. Drop the sentence, please,
and drop the "however," too.

Martin