On 09/13/2016 04:49 PM, John Ferlan wrote:
[...]
I took a peek at the v1, but suffice to say didn't follow it that closely since there's a number of changes in v2 related to patch ordering. You could get 3 different opinions on that matter too! Hopefully a consensus before too long. :)
yeah - v1 was lots of little steps that you were asked to combine. OTOH there are those that like to combine multiple things in one patch which can make it really difficult to follow. The order here was "followable" - my suggestion was more to try and get the guts (qemu) done first, then the fluff (domain/xml).
Agreed. v1 was perhaps overkill in breaking things down so small, because I was picking up an idea with a bit of dust on it.
Consider your patch order to be making progress towards some sustainable feature/bug fix - each step along the way being able to pass "make check syntax-check" so that future attempts to git bisect can point the fickle finger of fate (at someone else!). If a patch adds something that will be external that a followup patch will use - then great. If it's testable, even better. For example, adding virSCSIOpenVhost as a separate patch with the tests/qemuxml2argvmock.c is fine. One more tidbit here - it's a very faint memory, but check out tests/commandtest.c and the lengthy comment in mymain() regarding "expected" fd values. I think you're "safe" in what you've done - although your mock should mimic whatever changes you make to the primary function.
[...]
A wwpn doesn't generally have the "naa." prefix on it. So, why not just add that to the command line instead? That is a wwpn is a 16 hex digit value. I'm assuming that in order to support this feature a "naa." is prefixed and sent to qemu. Does that necessarily mean some other hypervisor would choose to place the "naa." prefix or would then code need to be added to strip it off for those other hypervisors? Looking at the qemu code - it seems fairly specific. Fairly certain that it's more a vhost thing of which the hypervisor utilizes. Per a recommendation made on this list years ago, I used targetcli to do the configuration rather than direct manipulation of sysfs, and according to its man page:
All WWNs are prefaced by their type, such as "iqn", "naa", or "ib", and may be given with or without colons.
I don't use targetcli so I'm not familiar with it.
Still not convinced using naa.# in the XML is the best way to go, but I'm not against it. I guess a lot depends on how the sysfs views things.
As to whether other hypervisors follow the same behavior, I do not know. I was just trying to conform to what it would allow in the case of vhost-scsi. Now, I suppose I'm confusing matters by saying "wwpn" here since as you say that's 16 hex digits, and should probably say just "wwn" ?
I guess I've always thought of a wwn/wwpn/wwnn as that string of bytes...
I always thought WWPN and WWNN as types of WWN...
Usually WWN/WWNN have been interchangeable, while WWPN is a WWN assigned to a port in a Fibre Channel fabric
...but have seen it this way also. Either way makes sense to me.
FWIW: virsh nodedev-dumpxml on a vport capable HBA and the created vHBA will list a wwnn, wwpn, and fabric_wwn. The fabric_wwn's will match. The wwnn/wwpn for the vport is what was used when running the HBA's/vport_create function. It's a tangled web. Wait until vGPU's show up (that's another black magic trick).
[...]
It would be nice to tell a user how to find that wwpn value via some command (virsh or otherwise) in order to fill in the wwpn attribute. Although there's always varying opinions on this since many times it's distro dependent. That's why I suggest virsh - at least if it's supportable we can display the wwpn there using nodedev-dumpxml. Hrm... nodedev-dumpxml is new to me, neat! I'll go down that bunny trail, but it seems like it'd be a bit of work to add the smarts for this.
Hours of all sorts of fun.... The 'nodedev-{list|dumpxml}' are essentially the libvirt mechanism to list/describe the various rabbit holes that is/are the sysfs tree and especially branches. The nodedev-list --tree will probably be of most interest, but so is virsh nodedev-list {scsi_host|pci|usb|scsi|scsi_generic|scs_target|net}
What/where is the 'vhost_scsi_wwpn'? IOW: It's not something defined yet - I'm assuming there's some output somewhere where that's pulled from. As near as I can tell, it's not anything that can be autogenerated off the bat. Somebody somewhere needs to first establish the sysfs entries for the vhost target, which can then be stored and loaded on a next (host) boot. The end result looks something like this:
AFAIK systemd handles the sysfs creation (ok at least for my purposes).
This is what I was concerned about - we really need a way to tell someone how to "easily" find the wwpn that's to be used. Although I don't have vhost-scsi on my laptop ;-)
Me neither. Of course, I'm using an s390, so that's a little different too. :)
- I have to figure that by using 'ls -al' on the path from a 'virsh nodedev-dumpxml' of a scsi_hostN that is an HBA will show a <path> that path would then be a starting point...
Googling "scsi_host HBA wwn" shows me some interesting results including ones that seem to indicate 'fc_host' is involved in some way even for the HBA (I guess that makes sense - where the vHBA is just an abstraction off of that). That said, a '/sys/class/fc_host/' is the starting point then to find things and virsh nodedev-list --cap vports would be the way to get capable HBA's.
/sys/class/fc_host is just showing the individual SCSI disks, but nothing that looks to point to the vhost-scsi target that I'm passing to QEMU. /sys/class/fc_vports is empty. (Oh wait, is NPIV turned on here? Ugh, pick this up later.)
[root@xxxxxxxx ~]# ls -l /sys/kernel/config/target/vhost/naa.50014056e4794195/tpgt_1/lun/lun_0/ total 0 Is the 'vhost' or 'vhost/naa.%s' linked from some /sys/class/{scsi|fc}_host/host%d directory?
No, I can't see anybody linking it from anywhere except the couple cross links in /sys/kernel/config/ shown below.
I think that becomes the key for traversal. Or maybe finding "5001...4195" in some other link from /sys/class/{scsi|fc}_host/....
lrwxrwxrwx 1 root root 0 Sep 13 15:44 752e150d11 -> ../../../../../../target/core/iblock_0/disk0 -rw-r--r-- 1 root root 4096 Sep 13 15:45 alua_tg_pt_gp -rw-r--r-- 1 root root 4096 Sep 13 15:45 alua_tg_pt_offline -rw-r--r-- 1 root root 4096 Sep 13 15:45 alua_tg_pt_status -rw-r--r-- 1 root root 4096 Sep 13 15:45 alua_tg_pt_write_md drwxr-xr-x 5 root root 0 Sep 13 15:44 statistics [root@xxxxxxxx ~]# cat /sys/kernel/config/target/core/iblock_0/disk0/info Status: ACTIVATED Max Queue Depth: 0 SectorSize: 512 HwMaxSectors: 4592 iBlock device: dm-0 UDEV PATH: /dev/mapper/mpathb readonly: 0 Major: 252 Minor: 0 CLAIMED: IBLOCK
(Aside: note the folder name in /sys/kernel/config that contains the wwn/wwpn name with the "naa." prefix.)
I see... Then again "/sys/kernel" isn't the path used by nodedev driver, hence my question above.
Erg, so then the nodedev stuff becomes bigger still.
One final question - what is this exposed as on the guest? "Some" scsi_hostM? For a SCSI LUN it's some 'sgN' value. I see the qemu side seems to infer some amount of scsi_generic code (read_naa_id), so it's mostly curiosity/learning for me. Anything that's put behind the target gets seen to the guest. So that can be one or more sgN LUNs and their sdX equivalents, and as near as I can tell a scsi_hostM as you describe. A quick test with two LUNs behind one vhost-scsi target gives me the following in the guest:
# ls -l /sys/bus/scsi/devices total 0 lrwxrwxrwx 1 root root 0 Sep 13 16:10 0:0:1:0 -> ../../../devices/css0/0.0.0002/0.0.beef/virtio2/host0/target0:0:1/0:0:1:0 lrwxrwxrwx 1 root root 0 Sep 13 16:10 0:0:1:1 -> ../../../devices/css0/0.0.0002/0.0.beef/virtio2/host0/target0:0:1/0:0:1:1 lrwxrwxrwx 1 root root 0 Sep 13 16:10 host0 -> ../../../devices/css0/0.0.0002/0.0.beef/virtio2/host0 lrwxrwxrwx 1 root root 0 Sep 13 16:11 target0:0:1 -> ../../../devices/css0/0.0.0002/0.0.beef/virtio2/host0/target0:0:1 # lsscsi [0:0:1:0] disk LIO-ORG disk0 4.0 /dev/sda [0:0:1:1] disk LIO-ORG disk1 4.0 /dev/sdb
FYI, hotplugging/unplugging devices to a target works just fine, except there's no notification that a guest can pick up on to know that a device had been added. Or worse, taken away.
Hmm. Maybe I'm just not asking the right question.
I guess my perception was that if you expose the 'scsi_hostN' HBA then you are giving the guest the whole HBA to use as it sees fit. The host shouldn't be touching the luns afterwards.
Correct, it's just not a "scsi_hostN" HBA that exists as part of the normal fibre attachment, but a vHBA that's defined to the vhost-scsi driver (via targetcli or some other manual handshaking).
What you've shown is 2 LUNS in the /sys/bus/scsi/ tree; however, is there a /sys/bus/scsi_host/hostN that corresponds to the passed through HBA?
Nope... (s390 host) # ls -l /sys/bus/ total 0 drwxr-xr-x 4 root root 0 Sep 13 23:12 ccw drwxr-xr-x 4 root root 0 Sep 13 23:12 clockevents drwxr-xr-x 4 root root 0 Sep 13 23:12 clocksource drwxr-xr-x 4 root root 0 Sep 13 23:12 container drwxr-xr-x 4 root root 0 Sep 13 23:12 cpu drwxr-xr-x 4 root root 0 Sep 13 23:12 css drwxr-xr-x 4 root root 0 Sep 13 23:12 etr drwxr-xr-x 4 root root 0 Sep 13 23:12 event_source drwxr-xr-x 4 root root 0 Sep 13 23:12 memory drwxr-xr-x 5 root root 0 Sep 13 23:12 pci drwxr-xr-x 4 root root 0 Sep 13 23:12 platform drwxr-xr-x 4 root root 0 Sep 13 23:12 scm drwxr-xr-x 4 root root 0 Sep 13 23:12 scsi drwxr-xr-x 4 root root 0 Sep 13 23:12 stp drwxr-xr-x 4 root root 0 Sep 13 23:12 virtio drwxr-xr-x 4 root root 0 Sep 13 23:12 workqueue
[...]
+ <attribute name="protocol"> + <choice> + <value>vhost</value> <!-- vhost, required --> + </choice> + </attribute> + <attribute name="wwpn"> + <data type="string"> + <param name="pattern">(naa\.)[0-9a-fA-F]{16}</param> + </data> If you go with the we'll add "naa." later, then you could just:
<ref name='wwn'/> This rings a bell with me, as to why I had the label be "wwpn" instead of "wwn" above. My attempt at trying to distinguish libvirt internals with something user-facing, maybe?
I think wwpn will still be technically correct (for some strange reason after skimming google results). The HBA will have a port which is how it's reached. I suppose the answer though is in how sysfs views things.
[...]
"pci", - "scsi") + "scsi", + "scsi_host") VIR_ENUM_IMPL(virDomainHostdevSubsysPCIBackend, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST, @@ -660,6 +661,10 @@ VIR_ENUM_IMPL(virDomainHostdevSubsysSCSIProtocol, "adapter", "iscsi") +VIR_ENUM_IMPL(virDomainHostdevSubsysHostProtocol, + VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_LAST, + "vhost") +
Notice how the virDomainHostdevSubsysSCSIProtocol adds 'adapter' first - this should do something similar (although the preference is to use "none" - I cannot recall why that was different). Because "none" == "adapter" in the case of SCSI?
Some sort of RNG magic in commit id '54ac483e6' - I forget the details (too long ago), but I think it had to do with being able to reuse the existing RNG without creating a whole new option.
Fair enough.
[...]
hmmm... If you're going to change this here, then qemuDomainAttachHostDevice would be changed at the same time. Hence why this ordering stuff to patches is important.
I cannot recall if removing would cause a build error in this case. It does, because of the switch statement not having a "default" case.
Perhaps a different ordering of patches removes the need.... But I know it's tough with this new type... I guess it's odd the *RemoveHostDevice needed it, but the corollary Attach didn't... Ahhh - I see why - the switch in Attach doesn't have the typecast (virDomainHostdevSubsysType).
I can send a patch for that or you can make the modification too...
I have a patch started, but kept it separate from this series because I got distracted/hoped to get v2 out before the last freeze. I will dust that off.
[...]
Thanks for the review. (Silent ACK on a lot of the above comments.) I'll try to get these all in place for a v3 with a little runway before the next freeze.
OK - hopefully I won't be neck deep in some other firefight
Thanks for taking a few minutes away from those to look over this little pile. I appreciate the fires that pop up and get in the way of the usual todo list. :)
and will be able to give them quicker attention. At the very least getting the capabilities changes in which is generally the most conflict...
Apologies for that. I've already rebased/resolved the capability conflict that occurred between posting to the list and now, and moved that to the head of my series. Taking a day's holiday tomorrow, so will get back to the v3 series Thursday. - Eric