[libvirt] [PATCH] storage: More uniquely identify NPIV LUNs

John Ferlan posted 1 patch 5 years, 3 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20181218224653.20286-1-jferlan@redhat.com
There is a newer version of this series
src/storage/storage_util.c | 61 +++++++++++++++++++++++++++++++++++++-
1 file changed, 60 insertions(+), 1 deletion(-)
[libvirt] [PATCH] storage: More uniquely identify NPIV LUNs
Posted by John Ferlan 5 years, 3 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1657468

Commit be1bb6c95 changed the way volumes were stored from a forward
linked list to a hash table. In doing so, it required that each vol
object would have 3 unique values as keys into tables - key, name,
and path. Due to how vHBA/NPIV LUNs are created/used this resulted
in a failure to utilize all the LUN's found during processing.

The virStorageBackendSCSINewLun uses virStorageBackendSCSISerial
in order to read/return the unique serial number of the LUN to be
used as a key for the volume.

However, for NPIV based LUNs the logic results in usage of the
same serial value for each path to the LUN. For example,

$ lsscsi -tg
...
[4:0:3:13]   disk    fc:0x207800c0ffd79b2a0xeb02ef   /dev/sde   /dev/sg16
[4:0:4:0]    disk    fc:0x50060169446021980xeb1f00   /dev/sdf   /dev/sg17
[4:0:5:0]    disk    fc:0x50060161446021980xeb2000   /dev/sdg   /dev/sg18
...

/lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sde
3600c0ff000d7a2965c603e5401000000
/lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdf
350060160c460219850060160c4602198
/lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdg
350060160c460219850060160c4602198

The /dev/sdf and /dev/sdg although separate LUNs would end up with the
same serial number used for the vol->key value. When attempting to add
the LUN via virStoragePoolObjAddVol, the hash table code will indicate
that we have a duplicate:

    virHashAddOrUpdateEntry:341 : internal error: Duplicate key

and thus the LUN is not added to the pool.

Digging deeper into the scsi_id output using the --export option one
will find that the only difference between the two luns is:

    ID_TARGET_PORT=1 for /dev/sdf
    ID_TARGET_PORT=2 for /dev/sdg

So, let's use the ID_TARGET_PORT string value along with the serial
to generate a more unique key using "@serial_PORT@target_port", where
@target_port is just the value of the ID_TARGET_PORT for the LUN.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/storage/storage_util.c | 61 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index a84ee5b600..d6d441c06d 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -3755,6 +3755,49 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
 }
 
 
+static char *
+virStorageBackendSCSITargetPort(const char *dev)
+{
+    char *target_port = NULL;
+    const char *id;
+#ifdef WITH_UDEV
+    virCommandPtr cmd = virCommandNewArgList(
+        "/lib/udev/scsi_id",
+        "--replace-whitespace",
+        "--whitelisted",
+        "--export",
+        "--device", dev,
+        NULL
+        );
+
+    /* Run the program and capture its output */
+    virCommandSetOutputBuffer(cmd, &target_port);
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
+#endif
+
+    if (target_port && STRNEQ(target_port, "") &&
+        (id = strstr(target_port, "ID_TARGET_PORT="))) {
+        char *nl = strchr(id, '\n');
+        if (nl)
+            *nl = '\0';
+        id = strrchr(id, '=');
+        memmove(target_port, id + 1, strlen(id));
+    } else {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unable to uniquely identify target port for '%s'"),
+                       dev);
+    }
+
+#ifdef WITH_UDEV
+ cleanup:
+    virCommandFree(cmd);
+#endif
+
+    return target_port;
+}
+
+
 static char *
 virStorageBackendSCSISerial(const char *dev)
 {
@@ -3813,6 +3856,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
     virStorageVolDefPtr vol = NULL;
     char *devpath = NULL;
+    char *key = NULL;
+    char *target_port = NULL;
     int retval = -1;
 
     /* Check if the pool is using a stable target path. The call to
@@ -3877,9 +3922,21 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
                                                  VIR_STORAGE_VOL_READ_NOERROR)) < 0)
         goto cleanup;
 
-    if (!(vol->key = virStorageBackendSCSISerial(vol->target.path)))
+    if (!(key = virStorageBackendSCSISerial(vol->target.path)))
         goto cleanup;
 
+    if (def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST &&
+        STRNEQ(key, vol->target.path)) {
+        /* NPIV based LUNs use the same "serial" key. In order to distinguish
+         * we need to append a port value */
+        if (!(target_port = virStorageBackendSCSITargetPort(vol->target.path)))
+            goto cleanup;
+        if (virAsprintf(&vol->key, "%s_PORT%s", key, target_port) < 0)
+            goto cleanup;
+    } else {
+        VIR_STEAL_PTR(vol->key, key);
+    }
+
     def->capacity += vol->target.capacity;
     def->allocation += vol->target.allocation;
 
@@ -3892,6 +3949,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
  cleanup:
     virStorageVolDefFree(vol);
     VIR_FREE(devpath);
+    VIR_FREE(target_port);
+    VIR_FREE(key);
     return retval;
 }
 
-- 
2.17.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: More uniquely identify NPIV LUNs
Posted by John Ferlan 5 years, 2 months ago
ping?

Tks,

John

On 12/18/18 5:46 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1657468
> 
> Commit be1bb6c95 changed the way volumes were stored from a forward
> linked list to a hash table. In doing so, it required that each vol
> object would have 3 unique values as keys into tables - key, name,
> and path. Due to how vHBA/NPIV LUNs are created/used this resulted
> in a failure to utilize all the LUN's found during processing.
> 
> The virStorageBackendSCSINewLun uses virStorageBackendSCSISerial
> in order to read/return the unique serial number of the LUN to be
> used as a key for the volume.
> 
> However, for NPIV based LUNs the logic results in usage of the
> same serial value for each path to the LUN. For example,
> 
> $ lsscsi -tg
> ...
> [4:0:3:13]   disk    fc:0x207800c0ffd79b2a0xeb02ef   /dev/sde   /dev/sg16
> [4:0:4:0]    disk    fc:0x50060169446021980xeb1f00   /dev/sdf   /dev/sg17
> [4:0:5:0]    disk    fc:0x50060161446021980xeb2000   /dev/sdg   /dev/sg18
> ...
> 
> /lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sde
> 3600c0ff000d7a2965c603e5401000000
> /lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdf
> 350060160c460219850060160c4602198
> /lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdg
> 350060160c460219850060160c4602198
> 
> The /dev/sdf and /dev/sdg although separate LUNs would end up with the
> same serial number used for the vol->key value. When attempting to add
> the LUN via virStoragePoolObjAddVol, the hash table code will indicate
> that we have a duplicate:
> 
>     virHashAddOrUpdateEntry:341 : internal error: Duplicate key
> 
> and thus the LUN is not added to the pool.
> 
> Digging deeper into the scsi_id output using the --export option one
> will find that the only difference between the two luns is:
> 
>     ID_TARGET_PORT=1 for /dev/sdf
>     ID_TARGET_PORT=2 for /dev/sdg
> 
> So, let's use the ID_TARGET_PORT string value along with the serial
> to generate a more unique key using "@serial_PORT@target_port", where
> @target_port is just the value of the ID_TARGET_PORT for the LUN.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/storage/storage_util.c | 61 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index a84ee5b600..d6d441c06d 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -3755,6 +3755,49 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
>  }
>  
>  
> +static char *
> +virStorageBackendSCSITargetPort(const char *dev)
> +{
> +    char *target_port = NULL;
> +    const char *id;
> +#ifdef WITH_UDEV
> +    virCommandPtr cmd = virCommandNewArgList(
> +        "/lib/udev/scsi_id",
> +        "--replace-whitespace",
> +        "--whitelisted",
> +        "--export",
> +        "--device", dev,
> +        NULL
> +        );
> +
> +    /* Run the program and capture its output */
> +    virCommandSetOutputBuffer(cmd, &target_port);
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +#endif
> +
> +    if (target_port && STRNEQ(target_port, "") &&
> +        (id = strstr(target_port, "ID_TARGET_PORT="))) {
> +        char *nl = strchr(id, '\n');
> +        if (nl)
> +            *nl = '\0';
> +        id = strrchr(id, '=');
> +        memmove(target_port, id + 1, strlen(id));
> +    } else {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unable to uniquely identify target port for '%s'"),
> +                       dev);
> +    }
> +
> +#ifdef WITH_UDEV
> + cleanup:
> +    virCommandFree(cmd);
> +#endif
> +
> +    return target_port;
> +}
> +
> +
>  static char *
>  virStorageBackendSCSISerial(const char *dev)
>  {
> @@ -3813,6 +3856,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>      virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>      virStorageVolDefPtr vol = NULL;
>      char *devpath = NULL;
> +    char *key = NULL;
> +    char *target_port = NULL;
>      int retval = -1;
>  
>      /* Check if the pool is using a stable target path. The call to
> @@ -3877,9 +3922,21 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>                                                   VIR_STORAGE_VOL_READ_NOERROR)) < 0)
>          goto cleanup;
>  
> -    if (!(vol->key = virStorageBackendSCSISerial(vol->target.path)))
> +    if (!(key = virStorageBackendSCSISerial(vol->target.path)))
>          goto cleanup;
>  
> +    if (def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST &&
> +        STRNEQ(key, vol->target.path)) {
> +        /* NPIV based LUNs use the same "serial" key. In order to distinguish
> +         * we need to append a port value */
> +        if (!(target_port = virStorageBackendSCSITargetPort(vol->target.path)))
> +            goto cleanup;
> +        if (virAsprintf(&vol->key, "%s_PORT%s", key, target_port) < 0)
> +            goto cleanup;
> +    } else {
> +        VIR_STEAL_PTR(vol->key, key);
> +    }
> +
>      def->capacity += vol->target.capacity;
>      def->allocation += vol->target.allocation;
>  
> @@ -3892,6 +3949,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>   cleanup:
>      virStorageVolDefFree(vol);
>      VIR_FREE(devpath);
> +    VIR_FREE(target_port);
> +    VIR_FREE(key);
>      return retval;
>  }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: More uniquely identify NPIV LUNs
Posted by John Ferlan 5 years, 2 months ago
ping^2?

Any brave soul want to take a look?

Tks -

John

On 1/5/19 9:56 AM, John Ferlan wrote:
> 
> ping?
> 
> Tks,
> 
> John
> 
> On 12/18/18 5:46 PM, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1657468
>>
>> Commit be1bb6c95 changed the way volumes were stored from a forward
>> linked list to a hash table. In doing so, it required that each vol
>> object would have 3 unique values as keys into tables - key, name,
>> and path. Due to how vHBA/NPIV LUNs are created/used this resulted
>> in a failure to utilize all the LUN's found during processing.
>>
>> The virStorageBackendSCSINewLun uses virStorageBackendSCSISerial
>> in order to read/return the unique serial number of the LUN to be
>> used as a key for the volume.
>>
>> However, for NPIV based LUNs the logic results in usage of the
>> same serial value for each path to the LUN. For example,
>>
>> $ lsscsi -tg
>> ...
>> [4:0:3:13]   disk    fc:0x207800c0ffd79b2a0xeb02ef   /dev/sde   /dev/sg16
>> [4:0:4:0]    disk    fc:0x50060169446021980xeb1f00   /dev/sdf   /dev/sg17
>> [4:0:5:0]    disk    fc:0x50060161446021980xeb2000   /dev/sdg   /dev/sg18
>> ...
>>
>> /lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sde
>> 3600c0ff000d7a2965c603e5401000000
>> /lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdf
>> 350060160c460219850060160c4602198
>> /lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdg
>> 350060160c460219850060160c4602198
>>
>> The /dev/sdf and /dev/sdg although separate LUNs would end up with the
>> same serial number used for the vol->key value. When attempting to add
>> the LUN via virStoragePoolObjAddVol, the hash table code will indicate
>> that we have a duplicate:
>>
>>     virHashAddOrUpdateEntry:341 : internal error: Duplicate key
>>
>> and thus the LUN is not added to the pool.
>>
>> Digging deeper into the scsi_id output using the --export option one
>> will find that the only difference between the two luns is:
>>
>>     ID_TARGET_PORT=1 for /dev/sdf
>>     ID_TARGET_PORT=2 for /dev/sdg
>>
>> So, let's use the ID_TARGET_PORT string value along with the serial
>> to generate a more unique key using "@serial_PORT@target_port", where
>> @target_port is just the value of the ID_TARGET_PORT for the LUN.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/storage/storage_util.c | 61 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
>> index a84ee5b600..d6d441c06d 100644
>> --- a/src/storage/storage_util.c
>> +++ b/src/storage/storage_util.c
>> @@ -3755,6 +3755,49 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
>>  }
>>  
>>  
>> +static char *
>> +virStorageBackendSCSITargetPort(const char *dev)
>> +{
>> +    char *target_port = NULL;
>> +    const char *id;
>> +#ifdef WITH_UDEV
>> +    virCommandPtr cmd = virCommandNewArgList(
>> +        "/lib/udev/scsi_id",
>> +        "--replace-whitespace",
>> +        "--whitelisted",
>> +        "--export",
>> +        "--device", dev,
>> +        NULL
>> +        );
>> +
>> +    /* Run the program and capture its output */
>> +    virCommandSetOutputBuffer(cmd, &target_port);
>> +    if (virCommandRun(cmd, NULL) < 0)
>> +        goto cleanup;
>> +#endif
>> +
>> +    if (target_port && STRNEQ(target_port, "") &&
>> +        (id = strstr(target_port, "ID_TARGET_PORT="))) {
>> +        char *nl = strchr(id, '\n');
>> +        if (nl)
>> +            *nl = '\0';
>> +        id = strrchr(id, '=');
>> +        memmove(target_port, id + 1, strlen(id));
>> +    } else {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                       _("unable to uniquely identify target port for '%s'"),
>> +                       dev);
>> +    }
>> +
>> +#ifdef WITH_UDEV
>> + cleanup:
>> +    virCommandFree(cmd);
>> +#endif
>> +
>> +    return target_port;
>> +}
>> +
>> +
>>  static char *
>>  virStorageBackendSCSISerial(const char *dev)
>>  {
>> @@ -3813,6 +3856,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>>      virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>>      virStorageVolDefPtr vol = NULL;
>>      char *devpath = NULL;
>> +    char *key = NULL;
>> +    char *target_port = NULL;
>>      int retval = -1;
>>  
>>      /* Check if the pool is using a stable target path. The call to
>> @@ -3877,9 +3922,21 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>>                                                   VIR_STORAGE_VOL_READ_NOERROR)) < 0)
>>          goto cleanup;
>>  
>> -    if (!(vol->key = virStorageBackendSCSISerial(vol->target.path)))
>> +    if (!(key = virStorageBackendSCSISerial(vol->target.path)))
>>          goto cleanup;
>>  
>> +    if (def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST &&
>> +        STRNEQ(key, vol->target.path)) {
>> +        /* NPIV based LUNs use the same "serial" key. In order to distinguish
>> +         * we need to append a port value */
>> +        if (!(target_port = virStorageBackendSCSITargetPort(vol->target.path)))
>> +            goto cleanup;
>> +        if (virAsprintf(&vol->key, "%s_PORT%s", key, target_port) < 0)
>> +            goto cleanup;
>> +    } else {
>> +        VIR_STEAL_PTR(vol->key, key);
>> +    }
>> +
>>      def->capacity += vol->target.capacity;
>>      def->allocation += vol->target.allocation;
>>  
>> @@ -3892,6 +3949,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>>   cleanup:
>>      virStorageVolDefFree(vol);
>>      VIR_FREE(devpath);
>> +    VIR_FREE(target_port);
>> +    VIR_FREE(key);
>>      return retval;
>>  }
>>  
>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: More uniquely identify NPIV LUNs
Posted by Erik Skultety 5 years, 2 months ago
On Tue, Jan 15, 2019 at 10:02:01AM -0500, John Ferlan wrote:
> ping^2?
>
> Any brave soul want to take a look?

I'll have a look tomorrow...and perhaps the day after that too :D...

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: More uniquely identify NPIV LUNs
Posted by Ján Tomko 5 years, 2 months ago
On Tue, Dec 18, 2018 at 05:46:53PM -0500, John Ferlan wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1657468
>
>Commit be1bb6c95 changed the way volumes were stored from a forward
>linked list to a hash table. In doing so, it required that each vol
>object would have 3 unique values as keys into tables - key, name,
>and path. Due to how vHBA/NPIV LUNs are created/used this resulted
>in a failure to utilize all the LUN's found during processing.
>
>The virStorageBackendSCSINewLun uses virStorageBackendSCSISerial
>in order to read/return the unique serial number of the LUN to be
>used as a key for the volume.
>
>However, for NPIV based LUNs the logic results in usage of the
>same serial value for each path to the LUN. For example,
>
>$ lsscsi -tg
>...
>[4:0:3:13]   disk    fc:0x207800c0ffd79b2a0xeb02ef   /dev/sde   /dev/sg16
>[4:0:4:0]    disk    fc:0x50060169446021980xeb1f00   /dev/sdf   /dev/sg17
>[4:0:5:0]    disk    fc:0x50060161446021980xeb2000   /dev/sdg   /dev/sg18
>...
>
>/lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sde
>3600c0ff000d7a2965c603e5401000000
>/lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdf
>350060160c460219850060160c4602198
>/lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdg
>350060160c460219850060160c4602198
>
>The /dev/sdf and /dev/sdg although separate LUNs would end up with the
>same serial number used for the vol->key value. When attempting to add
>the LUN via virStoragePoolObjAddVol, the hash table code will indicate
>that we have a duplicate:
>
>    virHashAddOrUpdateEntry:341 : internal error: Duplicate key
>
>and thus the LUN is not added to the pool.
>
>Digging deeper into the scsi_id output using the --export option one
>will find that the only difference between the two luns is:
>
>    ID_TARGET_PORT=1 for /dev/sdf
>    ID_TARGET_PORT=2 for /dev/sdg
>
>So, let's use the ID_TARGET_PORT string value along with the serial
>to generate a more unique key using "@serial_PORT@target_port", where
>@target_port is just the value of the ID_TARGET_PORT for the LUN.
>
>Signed-off-by: John Ferlan <jferlan@redhat.com>
>---
> src/storage/storage_util.c | 61 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 60 insertions(+), 1 deletion(-)
>
>diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
>index a84ee5b600..d6d441c06d 100644
>--- a/src/storage/storage_util.c
>+++ b/src/storage/storage_util.c
>@@ -3755,6 +3755,49 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
> }
>
>
>+static char *
>+virStorageBackendSCSITargetPort(const char *dev)
>+{
>+    char *target_port = NULL;
>+    const char *id;
>+#ifdef WITH_UDEV

This sort of conditional code within a function can lead to confusing
code. Also, why is this even depending on WITH_UDEV if it does not
depend on libudev at all?

>+    virCommandPtr cmd = virCommandNewArgList(
>+        "/lib/udev/scsi_id",
>+        "--replace-whitespace",
>+        "--whitelisted",
>+        "--export",
>+        "--device", dev,
>+        NULL
>+        );
>+
>+    /* Run the program and capture its output */

// THIS IS BRIDGE [0]

>+    virCommandSetOutputBuffer(cmd, &target_port);
>+    if (virCommandRun(cmd, NULL) < 0)
>+        goto cleanup;
>+#endif
>+
>+    if (target_port && STRNEQ(target_port, "") &&
>+        (id = strstr(target_port, "ID_TARGET_PORT="))) {
>+        char *nl = strchr(id, '\n');
>+        if (nl)
>+            *nl = '\0';
>+        id = strrchr(id, '=');
>+        memmove(target_port, id + 1, strlen(id));
>+    } else {
>+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+                       _("unable to uniquely identify target port for '%s'"),
>+                       dev);

The other usage of /lib/udev/scsi_id oddly guarded by WITH_UDEV actually
provides a fallback in case we're compiling on a system without udev.

Erroring out anyway makes the whole #ifdef even more pointless.

Jano

>+    }
>+
>+#ifdef WITH_UDEV
>+ cleanup:
>+    virCommandFree(cmd);
>+#endif
>+
>+    return target_port;
>+}
>+
>+

[0] https://www.abstrusegoose.com/432
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: More uniquely identify NPIV LUNs
Posted by John Ferlan 5 years, 2 months ago

On 1/15/19 11:32 AM, Ján Tomko wrote:
> On Tue, Dec 18, 2018 at 05:46:53PM -0500, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1657468
>>
>> Commit be1bb6c95 changed the way volumes were stored from a forward
>> linked list to a hash table. In doing so, it required that each vol
>> object would have 3 unique values as keys into tables - key, name,
>> and path. Due to how vHBA/NPIV LUNs are created/used this resulted
>> in a failure to utilize all the LUN's found during processing.
>>
>> The virStorageBackendSCSINewLun uses virStorageBackendSCSISerial
>> in order to read/return the unique serial number of the LUN to be
>> used as a key for the volume.
>>
>> However, for NPIV based LUNs the logic results in usage of the
>> same serial value for each path to the LUN. For example,
>>
>> $ lsscsi -tg
>> ...
>> [4:0:3:13]   disk    fc:0x207800c0ffd79b2a0xeb02ef   /dev/sde   /dev/sg16
>> [4:0:4:0]    disk    fc:0x50060169446021980xeb1f00   /dev/sdf   /dev/sg17
>> [4:0:5:0]    disk    fc:0x50060161446021980xeb2000   /dev/sdg   /dev/sg18
>> ...
>>
>> /lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sde
>> 3600c0ff000d7a2965c603e5401000000
>> /lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdf
>> 350060160c460219850060160c4602198
>> /lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdg
>> 350060160c460219850060160c4602198
>>
>> The /dev/sdf and /dev/sdg although separate LUNs would end up with the
>> same serial number used for the vol->key value. When attempting to add
>> the LUN via virStoragePoolObjAddVol, the hash table code will indicate
>> that we have a duplicate:
>>
>>    virHashAddOrUpdateEntry:341 : internal error: Duplicate key
>>
>> and thus the LUN is not added to the pool.
>>
>> Digging deeper into the scsi_id output using the --export option one
>> will find that the only difference between the two luns is:
>>
>>    ID_TARGET_PORT=1 for /dev/sdf
>>    ID_TARGET_PORT=2 for /dev/sdg
>>
>> So, let's use the ID_TARGET_PORT string value along with the serial
>> to generate a more unique key using "@serial_PORT@target_port", where
>> @target_port is just the value of the ID_TARGET_PORT for the LUN.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> src/storage/storage_util.c | 61 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
>> index a84ee5b600..d6d441c06d 100644
>> --- a/src/storage/storage_util.c
>> +++ b/src/storage/storage_util.c
>> @@ -3755,6 +3755,49 @@
>> virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
>> }
>>
>>
>> +static char *
>> +virStorageBackendSCSITargetPort(const char *dev)
>> +{
>> +    char *target_port = NULL;
>> +    const char *id;
>> +#ifdef WITH_UDEV
> 
> This sort of conditional code within a function can lead to confusing
> code. Also, why is this even depending on WITH_UDEV if it does not
> depend on libudev at all?
> 
>> +    virCommandPtr cmd = virCommandNewArgList(
>> +        "/lib/udev/scsi_id",
>> +        "--replace-whitespace",
>> +        "--whitelisted",
>> +        "--export",
>> +        "--device", dev,
>> +        NULL
>> +        );
>> +
>> +    /* Run the program and capture its output */
> 
> // THIS IS BRIDGE [0]
> 

cut-copy-paste and extend from virStorageBackendSCSISerial...

git blame back to original author leads to commit fdcd06ef7

The comment can easily be removed though.

>> +    virCommandSetOutputBuffer(cmd, &target_port);
>> +    if (virCommandRun(cmd, NULL) < 0)
>> +        goto cleanup;
>> +#endif
>> +
>> +    if (target_port && STRNEQ(target_port, "") &&
>> +        (id = strstr(target_port, "ID_TARGET_PORT="))) {
>> +        char *nl = strchr(id, '\n');
>> +        if (nl)
>> +            *nl = '\0';
>> +        id = strrchr(id, '=');
>> +        memmove(target_port, id + 1, strlen(id));
>> +    } else {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                       _("unable to uniquely identify target port for
>> '%s'"),
>> +                       dev);
> 
> The other usage of /lib/udev/scsi_id oddly guarded by WITH_UDEV actually
> provides a fallback in case we're compiling on a system without udev.

I honestly cannot imagine this working without udev. Whether having NPIV
volumes exposed "could" happen with node_device_hal is, well, highly
doubtful.  Maybe someone would be brave enough to remove the hal code.

Updating the existing virStorageBackendSCSISerial to parse output with
the --export option is possible, but affects more than just NPIV, so
this patch attempts to limit the scope.

> 
> Erroring out anyway makes the whole #ifdef even more pointless.

Well... If one doesn't error out then it leads to the problem seen
hidden in the weeds of the commit message, a:

    virHashAddOrUpdateEntry:341 : internal error: Duplicate key

is issued during Refresh processing (virStoragePoolFCRefreshThread)
which is (and must be) done in thread. It has to be done in a thread
because sync'ing with udev in order to "wait" for any NPIV LUNs to be
generated during storage pool creation is not an acceptable option
especially since we don't know how many exist.

So what we have now is any multiport volume (e.g. NPIV) having only one
volume (more than likely port=1) being reported/added to the storage
pool's volumes list.

If you'd prefer to see the same fallback as virStorageBackendSCSISerial,
then I'm not opposed to that, but I'm also not clear from your response
if that's what you'd prefer/expect/accept.

Please be more specific how you you'd like to see this code organized
such that you won't feel so lost in the weeds.  And yes, the processing
of an NPIV device is very much so like a Rube Goldberg project. Your
link also has a remarkable similarity to the mouse trap game
[https://en.wikipedia.org/wiki/Mouse_Trap_(game)] ;-).

Tks -

John

> 
> Jano
> 
>> +    }
>> +
>> +#ifdef WITH_UDEV
>> + cleanup:
>> +    virCommandFree(cmd);
>> +#endif
>> +
>> +    return target_port;
>> +}
>> +
>> +
> 
> [0] https://www.abstrusegoose.com/432

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: More uniquely identify NPIV LUNs
Posted by Ján Tomko 5 years, 2 months ago
On Tue, Jan 15, 2019 at 12:15:36PM -0500, John Ferlan wrote:
>
>
>On 1/15/19 11:32 AM, Ján Tomko wrote:
>> On Tue, Dec 18, 2018 at 05:46:53PM -0500, John Ferlan wrote:
>>> +    virCommandSetOutputBuffer(cmd, &target_port);
>>> +    if (virCommandRun(cmd, NULL) < 0)
>>> +        goto cleanup;
>>> +#endif
>>> +
>>> +    if (target_port && STRNEQ(target_port, "") &&
>>> +        (id = strstr(target_port, "ID_TARGET_PORT="))) {
>>> +        char *nl = strchr(id, '\n');
>>> +        if (nl)
>>> +            *nl = '\0';
>>> +        id = strrchr(id, '=');
>>> +        memmove(target_port, id + 1, strlen(id));
>>> +    } else {
>>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> +                       _("unable to uniquely identify target port for
>>> '%s'"),
>>> +                       dev);
>>
>> The other usage of /lib/udev/scsi_id oddly guarded by WITH_UDEV actually
>> provides a fallback in case we're compiling on a system without udev.
>
>I honestly cannot imagine this working without udev. Whether having NPIV
>volumes exposed "could" happen with node_device_hal is, well, highly
>doubtful.  Maybe someone would be brave enough to remove the hal code.
>

There is difference between a system with working udev (I assume that
would be equivalent to having /lib/udev/scsi_id) and a system with
WITH_UDEV, i.e. having devel headers for libudev.

A compile guard here is not necessary, since the virCommand APIs are
compiled unconditionally and I think the specific error message we get
(scsi_id command not found) is better than the vague error message.
Also, it's less code.

If you need to use #ifdef, conditionally compiling different functions
makes the control flow easier to follow.

>Updating the existing virStorageBackendSCSISerial to parse output with
>the --export option is possible, but affects more than just NPIV, so
>this patch attempts to limit the scope.
>
>>
>> Erroring out anyway makes the whole #ifdef even more pointless.
>
>Well... If one doesn't error out then it leads to the problem seen
>hidden in the weeds of the commit message, a:
>
>    virHashAddOrUpdateEntry:341 : internal error: Duplicate key
>

In the absence of the "WITH_UDEV" guards, the error would be reported
by virCommandRun.

>is issued during Refresh processing (virStoragePoolFCRefreshThread)
>which is (and must be) done in thread. It has to be done in a thread
>because sync'ing with udev in order to "wait" for any NPIV LUNs to be
>generated during storage pool creation is not an acceptable option
>especially since we don't know how many exist.
>
>So what we have now is any multiport volume (e.g. NPIV) having only one
>volume (more than likely port=1) being reported/added to the storage
>pool's volumes list.
>
>If you'd prefer to see the same fallback as virStorageBackendSCSISerial,
>then I'm not opposed to that, but I'm also not clear from your response
>if that's what you'd prefer/expect/accept.
>

In both cases (AFAIK we only build SCSI and ISCSI drivers on Linux),
I think the fallback is pointless due to the minimum of users that would
be using it.

Jano

>Please be more specific how you you'd like to see this code organized
>such that you won't feel so lost in the weeds.  And yes, the processing
>of an NPIV device is very much so like a Rube Goldberg project. Your
>link also has a remarkable similarity to the mouse trap game
>[https://en.wikipedia.org/wiki/Mouse_Trap_(game)] ;-).
>
>Tks -
>
>John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: More uniquely identify NPIV LUNs
Posted by John Ferlan 5 years, 2 months ago

On 1/17/19 8:45 AM, Ján Tomko wrote:
> On Tue, Jan 15, 2019 at 12:15:36PM -0500, John Ferlan wrote:
>>
>>
>> On 1/15/19 11:32 AM, Ján Tomko wrote:
>>> On Tue, Dec 18, 2018 at 05:46:53PM -0500, John Ferlan wrote:
>>>> +    virCommandSetOutputBuffer(cmd, &target_port);
>>>> +    if (virCommandRun(cmd, NULL) < 0)
>>>> +        goto cleanup;
>>>> +#endif
>>>> +
>>>> +    if (target_port && STRNEQ(target_port, "") &&
>>>> +        (id = strstr(target_port, "ID_TARGET_PORT="))) {
>>>> +        char *nl = strchr(id, '\n');
>>>> +        if (nl)
>>>> +            *nl = '\0';
>>>> +        id = strrchr(id, '=');
>>>> +        memmove(target_port, id + 1, strlen(id));
>>>> +    } else {
>>>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>>> +                       _("unable to uniquely identify target port for
>>>> '%s'"),
>>>> +                       dev);
>>>
>>> The other usage of /lib/udev/scsi_id oddly guarded by WITH_UDEV actually
>>> provides a fallback in case we're compiling on a system without udev.
>>
>> I honestly cannot imagine this working without udev. Whether having NPIV
>> volumes exposed "could" happen with node_device_hal is, well, highly
>> doubtful.  Maybe someone would be brave enough to remove the hal code.
>>
> 
> There is difference between a system with working udev (I assume that
> would be equivalent to having /lib/udev/scsi_id) and a system with
> WITH_UDEV, i.e. having devel headers for libudev.

I was merely pointing out, the relationship between existing code and
what I generated.

> 
> A compile guard here is not necessary, since the virCommand APIs are
> compiled unconditionally and I think the specific error message we get
> (scsi_id command not found) is better than the vague error message.
> Also, it's less code.

It's not that important that /scsi_id doesn't exist - that's not the
problem. The "STRNEQ(key, vol->target.path)" took care of that check
detail when adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST in the patch.

> 
> If you need to use #ifdef, conditionally compiling different functions
> makes the control flow easier to follow.
> 

I will change to a different model.

>> Updating the existing virStorageBackendSCSISerial to parse output with
>> the --export option is possible, but affects more than just NPIV, so
>> this patch attempts to limit the scope.
>>
>>>
>>> Erroring out anyway makes the whole #ifdef even more pointless.
>>
>> Well... If one doesn't error out then it leads to the problem seen
>> hidden in the weeds of the commit message, a:
>>
>>    virHashAddOrUpdateEntry:341 : internal error: Duplicate key
>>
> 
> In the absence of the "WITH_UDEV" guards, the error would be reported
> by virCommandRun.
> 

What error? Certainly not the duplicate key.

As the code stands now it's really fetching a duplicate key. Why this
worked before the hash tables is that the vol->key was duplicated in a
forward linked list and no one cared or knew unless they were trying to
get volumes by key in which key only 1 would return even though for NPIV
there were multiple LUN's with the same serial/key value. I have this
really vague recollection of a bug/problem mentioning this sometime long
ago, but cannot find the reference.

The WITH_UDEV is just there to follow the existing model of calling the
scsi_id.  The problem is that scsi_id without the --export only comes
back with the/a SERIAL_ID value that isn't unique enough for NPIV
volumes.  Maybe it'll help to "see" things in play...

# lsscsi -tg
...
[5:0:0:0]    enclosu fc:0x217800c0ffd79b2a,0x1001ef  -          /dev/sg19
[5:0:1:0]    enclosu fc:0x217000c0ffd79b2a,0x1002ef  -          /dev/sg20
[5:0:2:0]    enclosu fc:0x217800c0ffd7821d,0x1003ef  -          /dev/sg21
[5:0:3:0]    enclosu fc:0x217000c0ffd7821d,0x1004ef  -          /dev/sg22
[5:0:4:0]    disk    fc:0x5006016844602198,0x101f00  /dev/sdh   /dev/sg23
[5:0:5:0]    disk    fc:0x5006016044602198,0x102000  /dev/sdi   /dev/sg24
...

The processLU code will pass the /dev/sdh, /dev/sdi, (etc).  That get's
sent to the existing code to get the key (or serial), which returns:

#  /lib/udev/scsi_id --device /dev/sdh --whitelisted
--replace-whitespace /dev/sdh
350060160c460219850060160c4602198

#  /lib/udev/scsi_id --device /dev/sdh --whitelisted
--replace-whitespace /dev/sdi
350060160c460219850060160c4602198

...
See how they're the same - well when that volume is attempted to be
added in the Refresh thread later on, it gets dropped because of the
duplicate key.

W/ NPIV, the vHBA is found:

# virsh nodedev-list --cap fc_host
scsi_host3
scsi_host4
scsi_host5

# virsh nodedev-dumpxml scsi_host5
<device>
  <name>scsi_host5</name>

<path>/sys/devices/pci0000:00/0000:00:04.0/0000:10:00.0/host3/vport-3:0-0/host5</path>
  <parent>scsi_host3</parent>
  <capability type='scsi_host'>
    <host>5</host>
    <unique_id>2</unique_id>
    <capability type='fc_host'>
      <wwnn>5001a4a07a4ba8a1</wwnn>
      <wwpn>5001a4aced83bc53</wwpn>
      <fabric_wwn>2002000573de9a81</fabric_wwn>
    </capability>
  </capability>
</device>

>From which I create the vHBA:

# virsh pool-dumpxml poolvhba3
<pool type='scsi'>
  <name>poolvhba3</name>
...
    <adapter type='fc_host' parent='scsi_host3' managed='yes'
wwnn='5001a4a07a4ba8a1' wwpn='5001a4aced83bc53'/>
  </source>
...

And has the volumes listed (after RefreshThread runs):

# virsh vol-list poolvhba3
 Name         Path

------------------------------------------------------------------------------
 unit:0:4:0
/dev/disk/by-path/pci-0000:10:00.0-fc-0x5006016844602198-lun-0

# ls -al /dev/disk/by-path/pci-0000:10:00.0-fc-0x5006016844602198-lun-0
lrwxrwxrwx. 1 root root 9 Jan 16 13:49
/dev/disk/by-path/pci-0000:10:00.0-fc-0x5006016844602198-lun-0 -> ../../sdh

So, if I change to use --export I can get the ID_TARGET_PORT in order to
differentiate between the two:

# /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace
/dev/sdh --export
...
ID_SERIAL=350060160c460219850060160c4602198
...
ID_TARGET_PORT=2

# /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace
/dev/sdi --export
...
ID_SERIAL=350060160c460219850060160c4602198
...
ID_TARGET_PORT=2
...


Like pointed out before, it's walk through the weeds to get the answer.

>> is issued during Refresh processing (virStoragePoolFCRefreshThread)
>> which is (and must be) done in thread. It has to be done in a thread
>> because sync'ing with udev in order to "wait" for any NPIV LUNs to be
>> generated during storage pool creation is not an acceptable option
>> especially since we don't know how many exist.
>>
>> So what we have now is any multiport volume (e.g. NPIV) having only one
>> volume (more than likely port=1) being reported/added to the storage
>> pool's volumes list.
>>
>> If you'd prefer to see the same fallback as virStorageBackendSCSISerial,
>> then I'm not opposed to that, but I'm also not clear from your response
>> if that's what you'd prefer/expect/accept.
>>
> 
> In both cases (AFAIK we only build SCSI and ISCSI drivers on Linux),
> I think the fallback is pointless due to the minimum of users that would
> be using it.

Cannot disagree with this. I'm not sure who uses vHBA/NPIV. It seems to
be less and less important. Historically I recall two - HP and IBM and
maybe a few of their partners that were "active".

Still I think fixing this because it's a regression to list less LUNs
than previously is something that needs to be done.

I'll rework things to make the WITH_UDEV less apparent at least in the
storage_util code. Look up virStorageFileGetSCSIKey - it's similar, I'll
go from there.

John

> 
> Jano
> 
>> Please be more specific how you you'd like to see this code organized
>> such that you won't feel so lost in the weeds.  And yes, the processing
>> of an NPIV device is very much so like a Rube Goldberg project. Your
>> link also has a remarkable similarity to the mouse trap game
>> [https://en.wikipedia.org/wiki/Mouse_Trap_(game)] ;-).
>>
>> Tks -
>>
>> John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list