[libvirt] [PATCH] storage: More uniquely identify NPIV LUNs
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
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; }
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
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; +} + +
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; +} + +
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
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
participants (3)
-
Erik Skultety -
John Ferlan -
Ján Tomko