On 2/14/22 20:19, Martin Wilck wrote:
On Mon, 2022-02-14 at 19:55 +0100, Steffen Maier wrote:
There are also (FCP) HBAs that appear on a bus different from PCI.
Complements v0.6.0 commit
01ab2a468ea2 ("libmultipath: Add additional path wildcards").
With that we can easily get the full FCP addressing triplet
(HBA, WWPN, LUN) from multipath tools without additional tools
and correlation:
$ multipathd -k'show paths format "%w|%i|%a|%r"'
uuid |hcil |host adapter|target
WWPN
36005076400820293e8000000000000a0|1:0:3:160 |0.0.5080 |0x500507680b25c449
36005076400820293e8000000000000a0|1:0:4:160 |0.0.5080 |0x500507680b25c448
36005076400820293e8000000000000a0|58:0:3:160 |0.0.50c0 |0x500507680b26c449
36005076400820293e8000000000000a0|58:0:4:160 |0.0.50c0 |0x500507680b26c448
^^^^^^^^
instead of [undef]
As a side effect this patch theoretically also enables group by
host adapter for s390x based on v0.6.0 commit a28e61e5cc9a
("Crafted ordering of child paths for round robin path selector").
Reviewed-by: Benjamin Block <bblock@xxxxxxxxxxxxx>
Signed-off-by: Steffen Maier <maier@xxxxxxxxxxxxx>
---
Notes:
Changes since v1:
- Make sysfs_get_host_pci_name() static and generalize for adapters
on different bus types, in order to reduce code duplication (Ben).
The ancestor walk is always the same based on kernel driver core
with the only difference that PCI matches against driver name
whereas CCW matches against subsystem name.
Unfortunately, the diffstat increased because I had to move the
new static sysfs_get_host_bus_id() in front of its only user
sysfs_get_host_adapter_name() [or else a strange upfront prototype
would have been necessary].
libmultipath/discovery.c | 69 ++++++++++++++++++++++------------------
libmultipath/discovery.h | 1 -
2 files changed, 38 insertions(+), 32 deletions(-)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 7d939ae08004..5aba7e8d495f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
[...]
-
-int sysfs_get_host_pci_name(const struct path *pp, char *pci_name)
+static int sysfs_get_host_bus_id(const struct path *pp, char
*bus_id)
{
struct udev_device *hostdev, *parent;
char host_name[HOST_NAME_LEN];
- const char *driver_name, *value;
+ const char *driver_name, *subsystem_name, *value;
- if (!pp || !pci_name)
+ if (!pp || !bus_id)
return 1;
sprintf(host_name, "host%d", pp->sg_id.host_no);
Nit: While at it, you could have changed sprintf() to snprintf().
But this is no requirement, can be done separately / later.
@@ -525,10 +499,17 @@ int sysfs_get_host_pci_name(const struct path
*pp, char *pci_name)
}
if (!strcmp(driver_name, "pcieport"))
break;
The context doesn't show it here, but above these lines, we have
if (!driver_name) {
parent = udev_device_get_parent(parent);
continue;
}
Is it certain that this condition can't cause a valid ccw device (where
the driver attribute isn't required) to be skipped with the "continue"
statement? Even if the answer is "yes", I'd prefer self-explanatory
I had the same thought, but it does work. Apparently, the device node we're
interested in for ccw-attached FCP devices has both driver and subsystem
attributes that exist and both with a non-empty value. So we're good, even if
the preceding "early continue" skipped an uninteresting parent. However,
proving generality is beyond my capabilities, as I'm not even sure libudev
works on the udev database or sysfs directly. For instance,
# udevadm info -a /sys/class/scsi_host/host2
shows SUBSYSTEM and DRIVER property for each part of the ancestor chain, though
sometimes with empty string values which would not be a problem, whereas
# dir=$(readlink -e /sys/class/scsi_host/host2/); while [ -n "$dir" ]; do echo
$dir; ls -laF $dir/driver $dir/subsystem; dir=${dir%/*}; done
shows some ancestors completely lacking "driver" and some also lacking "subsystem".
code here, because not all of us are s390 / ccw experts.
I don't think there is anything specific to architecture or bus type.
In fact, I was surprised to see this code here to match for driver "pcieport"
[also "pci**e**port" sounds like PCI-Express, so what about HBAs attached to
the old parallel PCI instead of PCIe?], because udev-builtin-path_id.c looks
very consistent and similar between pci and ccw to me:
static int builtin_path_id(sd_device *dev, sd_netlink **rtnl, int argc, char
*argv[], bool test) {
/* walk up the chain of devices and compose path */
parent = dev;
while (parent) {
const char *subsys, *sysname;
if (sd_device_get_subsystem(parent, &subsys) < 0 ||
sd_device_get_sysname(parent, &sysname) < 0) {
;
} else if (streq(subsys, "pci")) {
path_prepend(&path, "pci-%s", sysname);
if (compat_path)
path_prepend(&compat_path, "pci-%s", sysname);
parent = skip_subsystem(parent, "pci");
supported_parent = true;
} else if (streq(subsys, "ccw")) {
path_prepend(&path, "ccw-%s", sysname);
if (compat_path)
path_prepend(&compat_path, "ccw-%s", sysname);
parent = skip_subsystem(parent, "ccw");
supported_transport = true;
supported_parent = true;
However, the details inside sd_device_get_subsystem() and
sd_device_get_sysname() are beyond my powers to understand, so there might be
differences hidden in there.
That said, I don't want to touch the PCI part here. I don't even have something
to test that.
Also, the code readability could be improved by changing the while loop
to a for loop and getting rid of the multiple
udev_device_get_parent(parent) calls. Like above, not a requirement,
but the change would be welcome.
+ subsystem_name = udev_device_get_subsystem(parent);
+ if (!subsystem_name) {
+ parent = udev_device_get_parent(parent);
+ continue;
+ }
+ if (!strcmp(subsystem_name, "ccw"))
+ break;
parent = udev_device_get_parent(parent);
}
if (parent) {
- /* pci_device found
+ /* pci_device or ccw fcp device found
*/
value = udev_device_get_sysname(parent);
@@ -537,7 +518,7 @@ int sysfs_get_host_pci_name(const struct path
*pp, char *pci_name)
return 1;
}
- strncpy(pci_name, value, SLOT_NAME_SIZE);
+ strncpy(bus_id, value, SLOT_NAME_SIZE);
Again not mandatory, but we should replace strncpy() by strlcpy() when
we encounter it.
Working on the other review comments. Stay tuned for v3.
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z and LinuxONE
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschaeftsfuehrung: David Faller
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
