Re: [dm-devel] [PATCH v2 1/2] libmultipath: support host adapter name lookup for s390x ccw bus

[Martin Wilck <martin.wilck@suse.com>]

On Tue, 2022-02-15 at 18:51 +0100, Steffen Maier wrote:
> On 2/14/22 20:19, Martin Wilck wrote:
> > 
> > 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.

I was thinking about something like this (untested):

for (parent = udev_device_get_parent(hostdev); parent; 
     parent = udev_device_get_parent(parent)) {
      driver_name = udev_device_get_driver(parent);
      if (driver_name && !strcmp(driver_name, "pcieport"))
               break;
      subsystem_name = udev_device_get_subsystem(parent);
      if (subsystem_name && !strcmp(subsystem_name, "ccw"))
               break;
}
if (!parent) {
       udev_device_unref(hostdev);
       return 1;
}
...
      

At least this would make it clear to the reader that we check for both
ccw and pcie on each level of the hierarchy. Functionally, it would be
equivalent.

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

I didn't mean you should. I was also wondering about "pcieport", too.
It dates back to a28e61e ("Crafted ordering of child paths for round
robin path selector"), which was merged before I started working on
multipath-tools. 

Indeed for this FC adapter: 

 /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.7/driver ->
      ../../../../bus/pci/drivers/qla2xxx

the "%a" wildcard returns '0000:00:01.0', which is the pcieport
upstream of the FC adapter, but arguably not the "host adapter" itself.
This feels wrong.  

@Ben, what's your take on this?

I suppose it may be related to the purpose of a28e61e which had nothing
to do with human-readable output. Rather, the patch attempted to
distribute IO evenly over paths, and apparently used the PCIe port to
identify the PCI-Express part of the "path". The %a wildcard was added
later.

See 
https://listman.redhat.com/archives/dm-devel/2014-February/msg00104.html

You may want to double-check if your CCW implementation meets the
purpose of commit a28e61e wrt distributing load evenly over adapters.

Thanks,
Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel