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@xxxxxxx>
> ---
> 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
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
