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

Thu Sep 24 06:52:40 UTC 2020

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

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