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(a)redhat.com&gt; > --- > 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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(a)redhat.com&gt; --- 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; +} + +