2:48 a.m.
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
5:05 a.m.
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 :|
5:41 a.m.
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
7:37 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:24 a.m.
New subject: [libvirt] [PATCH/RFC] qemu/kvm: allow to hot remove scsi/virtio disks
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
8:25 a.m.
New subject: [libvirt] [PATCH/RFC] qemu/kvm: allow to hot remove scsi/virtio disks
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:17 a.m.
New subject: [libvirt] [PATCH/RFC] qemu/kvm: allow to hot remove scsi/virtio disks
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
11:25 a.m.
New subject: [libvirt] [PATCH/RFC] qemu/kvm: allow to hot remove scsi/virtio disks
Hi Daniel,
On Tue, Oct 21, 2008 at 03:25:25PM +0200, Daniel Veillard wrote:
[..snip..]
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
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
9:33 a.m.
New subject: [libvirt] [PATCH/RFC] qemu/kvm: allow to hot remove scsi/virtio disks
On Thu, Oct 23, 2008 at 06:25:37PM +0200, Guido Günther wrote:
Hi Daniel,
On Tue, Oct 21, 2008 at 03:25:25PM +0200, Daniel Veillard wrote:
[..snip..]
> 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
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
8:42 a.m.
New subject: [libvirt] [PATCH/RFC] qemu/kvm: allow to hot remove scsi/virtio disks
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 :|
10:17 a.m.
New subject: [libvirt] [PATCH/RFC] qemu/kvm: allow to hot remove scsi/virtio disks
On Tue, Oct 21, 2008 at 02:42:40PM +0100, Daniel P. Berrange wrote:
[..snip..]
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
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
5868
days inactive
5880
days old
10 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Guido Günther