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