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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
