[PATCH] libmultipath: Allow discovery of USB devices

[PATCH] libmultipath: Allow discovery of USB devices

[Frank Meinl]

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.

Signed-off-by: Frank Meinl <frank.meinl@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;
 			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
+.RE
+.
+.
+.TP
 .B flush_on_last_del
 If set to
 .I yes
-- 
2.25.1

Re: [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@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

Re: [PATCH] libmultipath: Allow discovery of USB devices

[Benjamin Marzinski]

On Tue, Sep 22, 2020 at 09:59:36PM +0200, Martin Wilck wrote:
> 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.

I've actually seen other cases where people are using multipath on
single path devices just for the queuing, and when I thought about it, I
realized that it makes sense. Multipath works with devices as they are,
instead of needing special metadata, like lvm devices. People often
realize that they need this after the device is already set up. Plus,
multipath already deals with devices that have partitions or logical
volumes on top of them. It's also easy to configure exactly how you want
queueing to work. This use case might be a small nail, but it is a nail,
and multipath is a reasonable tool to get the job done.

It doesn't seem too hard to write a dm target that would queue and retry
IO at some configurable interval, for some configurable number of times,
when it failed. But you would also need to copy the work for getting the
device, and any partitons on it, to autoassemble after the frist time
it's set up and to make sure other layers use this device instead of the
underlying device. Or, people can just use multipath.

-Ben

> > 
> > Signed-off-by: Frank Meinl <frank.meinl@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
> 

Re: [PATCH] libmultipath: Allow discovery of USB devices

[Martin Wilck]

On Tue, 2020-09-22 at 17:27 -0500, Benjamin Marzinski wrote:
> On Tue, Sep 22, 2020 at 09:59:36PM +0200, Martin Wilck wrote:
> > 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.
> 
> I've actually seen other cases where people are using multipath on
> single path devices just for the queuing, and when I thought about
> it, I
> realized that it makes sense. Multipath works with devices as they
> are,
> instead of needing special metadata, like lvm devices. People often
> realize that they need this after the device is already set up. Plus,
> multipath already deals with devices that have partitions or logical
> volumes on top of them. It's also easy to configure exactly how you
> want
> queueing to work. This use case might be a small nail, but it is a
> nail,
> and multipath is a reasonable tool to get the job done.
> 
> It doesn't seem too hard to write a dm target that would queue and
> retry
> IO at some configurable interval, for some configurable number of
> times,
> when it failed. But you would also need to copy the work for getting
> the
> device, and any partitons on it, to autoassemble after the frist time
> it's set up and to make sure other layers use this device instead of
> the
> underlying device. Or, people can just use multipath.

Ok. So Frank, please just clarify the remaining minor points. You may
actually want to put (a short version of) this motivation in the man
page.

Regards,
Martin

Re: [PATCH] libmultipath: Allow discovery of USB devices

[Frank Meinl]

On 22.09.20 21:59, Martin Wilck wrote:
> 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.

During my search for a tool, which allows to queue I/O if a device 
disappears, I checked all existing device-mapper modules. Maybe I'm 
wrong, but multipath was the only which had already everything on-board. 
Furthermore, another big advantage are the really sophisticated 
userspace tools. In fact, the tricky part is not the queuing itself, but 
the reconnect event. You have to notice when a new device appears, then 
you have to check if it's the previously disconnected device, and 
finally you have to tell the device-mapper to reroute I/O again and to 
flush the queue...
After all, I thought it would be better to misuse multipath for this, 
than to reinvent the wheel ;)

> 
> Waiting for others' opinions.
> 
>>
>> Signed-off-by: Frank Meinl <frank.meinl@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.
> 

You are correct! I checked with the kernel drivers and it seems that the 
"old" usb bulk storage driver also uses the SCSI subsystem of linux. So 
SCSI_PROTOCOL_USB would definitely be more appropriate here.

>>   			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.

Will do, no problem!

> 
> Martin
> 
> 

Regards,
Frank

Re: [PATCH] libmultipath: Allow discovery of USB devices

[Frank Meinl]

Hello,

thank you all for your replies.
And of course good to hear that you are also open for this rather 
special use case ;)

I will then revise the patch as you suggested, and send it again for review.

   Frank

On 23.09.20 00:34, Martin Wilck wrote:
> On Tue, 2020-09-22 at 17:27 -0500, Benjamin Marzinski wrote:
>> On Tue, Sep 22, 2020 at 09:59:36PM +0200, Martin Wilck wrote:
>>> 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.
>>
>> I've actually seen other cases where people are using multipath on
>> single path devices just for the queuing, and when I thought about
>> it, I
>> realized that it makes sense. Multipath works with devices as they
>> are,
>> instead of needing special metadata, like lvm devices. People often
>> realize that they need this after the device is already set up. Plus,
>> multipath already deals with devices that have partitions or logical
>> volumes on top of them. It's also easy to configure exactly how you
>> want
>> queueing to work. This use case might be a small nail, but it is a
>> nail,
>> and multipath is a reasonable tool to get the job done.
>>
>> It doesn't seem too hard to write a dm target that would queue and
>> retry
>> IO at some configurable interval, for some configurable number of
>> times,
>> when it failed. But you would also need to copy the work for getting
>> the
>> device, and any partitons on it, to autoassemble after the frist time
>> it's set up and to make sure other layers use this device instead of
>> the
>> underlying device. Or, people can just use multipath.
> 
> Ok. So Frank, please just clarify the remaining minor points. You may
> actually want to put (a short version of) this motivation in the man
> page.
> 
> Regards,
> Martin
> 
>