[dm-devel] [PATCH v2 0/2] multipath-tools: FCP addressing display support (for s390x)

[dm-devel] [PATCH v2 0/2] multipath-tools: FCP addressing display support (for s390x)

[Steffen Maier]

It is quicker and easier than writing some script to correlate
multipath output with other query tooling for FCP addressing.

For patch set version ChangeLog, see indidivual patches.

Steffen Maier (2):
  libmultipath: support host adapter name lookup for s390x ccw bus
  libmultipath: add %L path wildcard for 64-bit hex LUN

 libmultipath/discovery.c | 69 ++++++++++++++++++++++------------------
 libmultipath/discovery.h |  1 -
 libmultipath/print.c     | 20 ++++++++++++
 3 files changed, 58 insertions(+), 32 deletions(-)


base-commit: 70602364987e27739b5521d94d684993d877157a
-- 
2.27.0

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

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

[Steffen Maier]

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@linux.ibm.com>
Signed-off-by: Steffen Maier <maier@linux.ibm.com>
---

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
@@ -475,39 +475,13 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
 	return 0;
 }
 
-int sysfs_get_host_adapter_name(const struct path *pp, char *adapter_name)
-{
-	int proto_id;
-
-	if (!pp || !adapter_name)
-		return 1;
-
-	proto_id = pp->sg_id.proto_id;
-
-	if (proto_id != SCSI_PROTOCOL_FCP &&
-	    proto_id != SCSI_PROTOCOL_SAS &&
-	    proto_id != SCSI_PROTOCOL_ISCSI &&
-	    proto_id != SCSI_PROTOCOL_SRP) {
-		return 1;
-	}
-	/* iscsi doesn't have adapter info in sysfs
-	 * get ip_address for grouping paths
-	 */
-	if (pp->sg_id.proto_id == SCSI_PROTOCOL_ISCSI)
-		return sysfs_get_iscsi_ip_address(pp, adapter_name);
-
-	/* fetch adapter pci name for other protocols
-	 */
-	return sysfs_get_host_pci_name(pp, adapter_name);
-}
-
-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);
@@ -525,10 +499,17 @@ int sysfs_get_host_pci_name(const struct path *pp, char *pci_name)
 		}
 		if (!strcmp(driver_name, "pcieport"))
 			break;
+		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);
 		udev_device_unref(hostdev);
 		return 0;
 	}
@@ -545,6 +526,32 @@ int sysfs_get_host_pci_name(const struct path *pp, char *pci_name)
 	return 1;
 }
 
+int sysfs_get_host_adapter_name(const struct path *pp, char *adapter_name)
+{
+	int proto_id;
+
+	if (!pp || !adapter_name)
+		return 1;
+
+	proto_id = pp->sg_id.proto_id;
+
+	if (proto_id != SCSI_PROTOCOL_FCP &&
+	    proto_id != SCSI_PROTOCOL_SAS &&
+	    proto_id != SCSI_PROTOCOL_ISCSI &&
+	    proto_id != SCSI_PROTOCOL_SRP) {
+		return 1;
+	}
+	/* iscsi doesn't have adapter info in sysfs
+	 * get ip_address for grouping paths
+	 */
+	if (pp->sg_id.proto_id == SCSI_PROTOCOL_ISCSI)
+		return sysfs_get_iscsi_ip_address(pp, adapter_name);
+
+	/* fetch adapter bus-ID for other protocols
+	 */
+	return sysfs_get_host_bus_id(pp, adapter_name);
+}
+
 int sysfs_get_iscsi_ip_address(const struct path *pp, char *ip_address)
 {
 	struct udev_device *hostdev;
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index 095657bb9de4..466af34504de 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -44,7 +44,6 @@ int store_pathinfo (vector pathvec, struct config *conf,
 		    struct path **pp_ptr);
 int sysfs_set_scsi_tmo (struct multipath *mpp, unsigned int checkint);
 int sysfs_get_timeout(const struct path *pp, unsigned int *timeout);
-int sysfs_get_host_pci_name(const struct path *pp, char *pci_name);
 int sysfs_get_iscsi_ip_address(const struct path *pp, char *ip_address);
 int sysfs_get_host_adapter_name(const struct path *pp,
 				char *adapter_name);
-- 
2.27.0

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

[dm-devel] [PATCH v2 2/2] libmultipath: add %L path wildcard for 64-bit hex LUN

[Steffen Maier]

Complements v0.6.0 commit
01ab2a468ea2 ("libmultipath: Add additional path wildcards") as well as
("libmultipath: support host adapter name lookup for s390x ccw bus").

With that we can easily get the full FCP addressing triplet
(HBA, WWPN, FCPLUN) from multipath tools without additional tools
and correlation:

$ multipathd -k'show paths format "%w|%a|%r|%L"'
uuid                             |host adapter|target WWPN       |LUN hex
36005076400820293e8000000000000a0|0.0.5080    |0x500507680b25c449|0x00a0000000000000
36005076400820293e8000000000000a0|0.0.5080    |0x500507680b25c448|0x00a0000000000000
36005076400820293e8000000000000a0|0.0.50c0    |0x500507680b26c449|0x00a0000000000000
36005076400820293e8000000000000a0|0.0.50c0    |0x500507680b26c448|0x00a0000000000000

Likewise, add a field lun_hex for JSON path output.

Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Steffen Maier <maier@linux.ibm.com>
---

Notes:
    Changes since v1:
    - added 2 Reviewed-by (Ben, Martin)

 libmultipath/print.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index 221b515f23d3..4f6146e85fc9 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -96,6 +96,7 @@
 			     "            \"host_wwpn\" : \"%R\",\n" \
 			     "            \"target_wwpn\" : \"%r\",\n" \
 			     "            \"host_adapter\" : \"%a\",\n" \
+			     "            \"lun_hex\" : \"%L\",\n" \
 			     "            \"marginal_st\" : \"%M\""
 
 #define PROGRESS_LEN  10
@@ -451,6 +452,24 @@ snprint_hcil (struct strbuf *buff, const struct path * pp)
 			pp->sg_id.lun);
 }
 
+
+static int
+snprint_path_lunhex (struct strbuf *buff, const struct path * pp)
+{
+	uint64_t lunhex = SCSI_INVALID_LUN, scsilun;
+
+	if (!pp || pp->sg_id.host_no < 0)
+		return print_strbuf(buff, "0x%016" PRIx64, lunhex);
+
+	scsilun = pp->sg_id.lun;
+	/* cf. Linux kernel function int_to_scsilun() */
+	lunhex = ((scsilun & 0x000000000000ffffULL) << 48) |
+		((scsilun & 0x00000000ffff0000ULL) << 16) |
+		((scsilun & 0x0000ffff00000000ULL) >> 16) |
+		((scsilun & 0xffff000000000000ULL) >> 48);
+	return print_strbuf(buff, "0x%016" PRIx64, lunhex);
+}
+
 static int
 snprint_dev (struct strbuf *buff, const struct path * pp)
 {
@@ -842,6 +861,7 @@ static const struct path_data pd[] = {
 	{'0', "failures",      snprint_path_failures},
 	{'P', "protocol",      snprint_path_protocol},
 	{'I', "init_st",       snprint_initialized},
+	{'L', "LUN hex",       snprint_path_lunhex},
 };
 
 static const struct pathgroup_data pgd[] = {
-- 
2.27.0

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

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

[Martin Wilck]

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@linux.ibm.com>
> Signed-off-by: Steffen Maier <maier@linux.ibm.com>
> ---
> 
> 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
code here, because not all of us are s390 / ccw experts.

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.

Regards,
Martin


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

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

[Steffen Maier]

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@linux.ibm.com>
>> Signed-off-by: Steffen Maier <maier@linux.ibm.com>
>> ---
>>
>> 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@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Martin Wilck]

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

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

[Martin Wilck]

On Tue, 2022-02-15 at 21:38 +0100, Martin Wilck wrote:
> 
> 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;
> }
> ...

So you did exactly that in your v3. Nice!

Martin


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