[libvirt] [PATCH] qemu/kvm: allow to hotadd scsi/virtio disks
Hi, recent kvm allows to hot add scsi/virtio disks, it uses pci hotplugging for that. Attached patch adds support for this to libvirt. -- Guido
On Fri, Oct 17, 2008 at 09:48:58AM +0200, Guido G?nther wrote:
Hi, recent kvm allows to hot add scsi/virtio disks, it uses pci hotplugging for that. Attached patch adds support for this to libvirt.
Great, I didn't know SCSI allowed hotplug in QEMU. ACK to the patch. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Oct 17, 2008 at 11:05:50AM +0100, Daniel P. Berrange wrote:
On Fri, Oct 17, 2008 at 09:48:58AM +0200, Guido G?nther wrote:
Hi, recent kvm allows to hot add scsi/virtio disks, it uses pci hotplugging for that. Attached patch adds support for this to libvirt.
Great, I didn't know SCSI allowed hotplug in QEMU. Even NICs are hot pluggable now. And since qemu prints a reasonable OK message we can parse the PCI slot number from the reply which allows for unplugging as well (delayed that since I'm a bit short on time).
My patch contained a tiny error: -+ safe_path = qemudEscapeShellArg(dev->data.disk->src); ++ safe_path = qemudEscapeMonitorArg(dev->data.disk->src); corrected version attached, -- Guido
On Fri, Oct 17, 2008 at 12:41:14PM +0200, Guido Günther wrote:
On Fri, Oct 17, 2008 at 11:05:50AM +0100, Daniel P. Berrange wrote:
On Fri, Oct 17, 2008 at 09:48:58AM +0200, Guido G?nther wrote:
Hi, recent kvm allows to hot add scsi/virtio disks, it uses pci hotplugging for that. Attached patch adds support for this to libvirt.
Great, I didn't know SCSI allowed hotplug in QEMU. Even NICs are hot pluggable now. And since qemu prints a reasonable OK message we can parse the PCI slot number from the reply which allows for unplugging as well (delayed that since I'm a bit short on time).
My patch contained a tiny error:
-+ safe_path = qemudEscapeShellArg(dev->data.disk->src); ++ safe_path = qemudEscapeMonitorArg(dev->data.disk->src);
corrected version attached,
Looks fine, i just removed a couple of extra spaces at end of line before commiting :-) thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Oct 17, 2008 at 02:37:17PM +0200, Daniel Veillard wrote: [..snip..]
Looks fine, i just removed a couple of extra spaces at end of line before commiting :-) Thanks a lot for applying this so quickly! Attached is a patch that allows for unplugging of disks. To do that I added a token to virDomainDiskDef that holds the PCI bus/slot number [1] so we can indentify the device on unplug. It's a union since other busses/hypervisors might have other requirements. The token is meant as an internal handle and will never show up in an XML and should probably move up into virDomainDeviceDef. Comments are welcome. -- Guido
[1] bus is always == 0 at the moment
On Fri, Oct 17, 2008 at 04:24:52PM +0200, Guido Günther wrote:
On Fri, Oct 17, 2008 at 02:37:17PM +0200, Daniel Veillard wrote: [..snip..]
Looks fine, i just removed a couple of extra spaces at end of line before commiting :-) Thanks a lot for applying this so quickly! Attached is a patch that allows for unplugging of disks. To do that I added a token to virDomainDiskDef that holds the PCI bus/slot number [1] so we can indentify the device on unplug. It's a union since other busses/hypervisors might have other requirements. The token is meant as an internal handle and will never show up in an XML and should probably move up into virDomainDeviceDef. Comments are welcome.
In principle this looks just fine to me except a couple of stylistic issues: [...]
+++ b/src/domain_conf.h @@ -84,6 +84,12 @@ struct _virDomainDiskDef { int type; int device; int bus; + union { + struct { + int bus; + int slot; + } pci; + } token;
I'm not sure what the point of the token union is. Other adressing mechanism may be used still I think having the structure not unioned at this point makes things a bit clearer.
+static int qemudDomainDetchPciDiskDevice(virDomainPtr dom, virDomainDeviceDefPtr dev)
Well I don't really see an improvement in using "Detch" instead of "Detach", it's not like the identifier is much smaller, makes it less readable IMHO, ditto for qemudDomainDetchDevice() [...]
+ if (detach->token.pci.slot < 1 || detach->token.pci.bus != 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s cannot be detached"), detach->dst); + return -1; + }
shouldn't the error be "non PCI disk %s cannot be detached" instead ?
+ ret = asprintf(&cmd, "pci_del 0 %d", detach->token.pci.slot); + if (ret == -1) { + qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + return ret; + } + + if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot detach disk %s"), detach->dst);
Live attach/detach operations are among the ones the harder to debug from an user perspective I would suggest to try to be as explicit as possible "failed to execute detach disk %s command"
+ VIR_FREE(cmd); + return -1; + } + + DEBUG ("pci_del reply: %s", reply); + /* If the command fails due to a wrong slot qemu prints: invalid slot, + * nothing is printed on success */ + if (strstr(reply, "invalid slot")) { + qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot detach disk %s"), detach->dst);
same thing if we have a hint about why this failed ... "cannot detach disk %s : invalid slot" [...]
+static int qemudDomainDetchDevice(virDomainPtr dom, + const char *xml) {
Detach :-)
+ if (dev->type == VIR_DOMAIN_DEVICE_DISK && + dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && + (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || + dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) + ret = qemudDomainDetchPciDiskDevice(dom, dev); + else { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, + "%s", _("this device type cannot be attached"));
or even better turn this positively as "only SCSI or virtio disk device can be attached dynamically" Those are just stylistic issues, I can apply the patch with those changed if you wish if you don't have time for a new patch, thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Hi Daniel, On Tue, Oct 21, 2008 at 03:25:25PM +0200, Daniel Veillard wrote:
Those are just stylistic issues, I can apply the patch with those changed if you wish if you don't have time for a new patch, I'll come up with a corrected version, no problem. -- Guido
Those are just stylistic issues, I can apply the patch with those changed if you wish if you don't have time for a new patch, Thanks for your comments. Updated version attached. I basically removed
Hi Daniel, On Tue, Oct 21, 2008 at 03:25:25PM +0200, Daniel Veillard wrote: [..snip..] the union in favour of a single slotnum variable and cleaned up the error messages. As Daniel P. pointed out, the situation isn't ideal since we can only hotremove disks added during the same session since qemu has no real notion of "reidentifying" the disks that were passed on the command line, this can hopefully be resolved in the future with a little help from qemu. Cheers, -- Guido
On Thu, Oct 23, 2008 at 06:25:37PM +0200, Guido Günther wrote:
Those are just stylistic issues, I can apply the patch with those changed if you wish if you don't have time for a new patch, Thanks for your comments. Updated version attached. I basically removed
Hi Daniel, On Tue, Oct 21, 2008 at 03:25:25PM +0200, Daniel Veillard wrote: [..snip..] the union in favour of a single slotnum variable and cleaned up the error messages. As Daniel P. pointed out, the situation isn't ideal since we can only hotremove disks added during the same session since qemu has no real notion of "reidentifying" the disks that were passed on the command line, this can hopefully be resolved in the future with a little help from qemu.
Oops I had forgotten about that patch !
Cheers, [...] +static int qemudDomainDetachDevice(virDomainPtr dom, + const char *xml) { + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid); + virDomainDeviceDefPtr dev; + int ret = 0; + + if (!vm) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + "%s", _("no domain with matching uuid")); + return -1; + } + + if (!virDomainIsActive(vm)) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot attach device on inactive domain")); + return -1; + } + + dev = virDomainDeviceDefParse(dom->conn, vm->def, xml); + if (dev == NULL) { + return -1; + }
Applied, I just had to modify the above function since the virDomainDeviceDefParse no take the extra driver->caps argument. Commited, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Oct 17, 2008 at 04:24:52PM +0200, Guido G?nther wrote:
On Fri, Oct 17, 2008 at 02:37:17PM +0200, Daniel Veillard wrote: [..snip..]
Looks fine, i just removed a couple of extra spaces at end of line before commiting :-) Thanks a lot for applying this so quickly! Attached is a patch that allows for unplugging of disks. To do that I added a token to virDomainDiskDef that holds the PCI bus/slot number [1] so we can indentify the device on unplug. It's a union since other busses/hypervisors might have other requirements. The token is meant as an internal handle and will never show up in an XML and should probably move up into virDomainDeviceDef. Comments are welcome.
The thing I'm not happy about with this scheme, is that it only works if you previously hot-attached the disk during the lifetime of this particular execution of the VM. If you had a disk specified on the command line with -driver, then it is not able to unplug it. Having a detach command that may work or may fail depending on how the disk was previously configured is not very nice. Is there not some 'info pci' or 'info disk' command we cna use to find out the PCI slot number at the time we want to detach the device. This would make it work for all disks, and avoid the need to track the state in that virDomainDiskDef union Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Is there not some 'info pci' or 'info disk' command we cna use to find out the PCI slot number at the time we want to detach the device. This would make it work for all disks, and avoid the need to track the state in that virDomainDiskDef union I don't like restricting this to hot added disks either but for one this will probably be a very common suitation and more important qemu just doesn't give enough information to do this reliably. The only really reliable way would (at least I think) be for the user (in this case libvirt) to pass a token to qemu (with -drive and with
On Tue, Oct 21, 2008 at 02:42:40PM +0100, Daniel P. Berrange wrote: [..snip..] pci_add). We should then be able to detach using this token. Something else will always be dependent on what hardware qemu currently emulates. This would need to be implemented in qemu for usb and pci. -- Guido
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Guido Günther