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; +} + +