Consider a guest started with the following XML snippet: <controller type='scsi' model='virtio-scsi' index='0'/> <hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='8' unit='1074151456'/>

</source> </hostdev> If we attach/create a virtio-scsi hostdev device but forget to include a virtio-scsi controller, one is silently created for us. (See qemuDomainFindOrCreateSCSIDiskController for context.)

Detaching the hostdev, followed by the controller, works well and the guest behaves appropriately.

If we detach the virtio-scsi controller device first, Libvirt silently detaches any associated hostdevs for us too. But all is not well, as the guest is unable to receive new virtio-scsi devices (the attach commands succeed, but devices never appear within the guest), nor even be shutdown, after this point.

It appears that something is tied up in the host virtio layer.

While I investigate this, we can see if a controller is being used by any hostdevs, and prevent the detach if it would lead us down this path.

$ virsh detach-device guest_one_virtio_scsi scsicontroller.xml error: Failed to detach device from scsicontroller.xml error: operation failed: device cannot be detached: device is busy $ virsh detach-device guest_one_virtio_scsi scsidisk.xml Device detached successfully $ virsh detach-device guest_one_virtio_scsi scsicontroller.xml Device detached successfully Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_hotplug.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b03e618..5db7abf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4443,6 +4443,7 @@ static bool qemuDomainDiskControllerIsBusy(virDomainObjPtr vm, { size_t i; virDomainDiskDefPtr disk; + virDomainHostdevDefPtr hostdev; for (i = 0; i < vm->def->ndisks; i++) { disk = vm->def->disks[i]; @@ -4465,6 +4466,15 @@ static bool qemuDomainDiskControllerIsBusy(virDomainObjPtr vm, return true; } + for (i = 0; i < vm->def->nhostdevs; i++) { + hostdev = vm->def->hostdevs[i]; + if (!virHostdevIsSCSIDevice(hostdev) || + detach->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + continue; + if (hostdev->info->addr.drive.controller == detach->idx) + return true; + } +

return false; }

>> If we attach/create a virtio-scsi hostdev device but forget to include >> a virtio-scsi controller, one is silently created for us. (See >> qemuDomainFindOrCreateSCSIDiskController for context.) > So is this attach/create done to a guest with or without the above > controller? Is the XML using a different controller (perhaps I'm > reading too much into the example ;-)) Haha, perhaps I tried to combine my example too much. In the above paragraph, the controller tag is not present. > >> Detaching the hostdev, followed by the controller, works well and the >> guest behaves appropriately. > OK I guess I'd expect that order to work. > >> If we detach the virtio-scsi controller device first, Libvirt silently >> detaches any associated hostdevs for us too. But all is not well, >> as the guest is unable to receive new virtio-scsi devices (the attach >> commands succeed, but devices never appear within the guest), nor even >> be shutdown, after this point. > I think this is where you lost me... Where does libvirt silently detach > the associated hostdevs? qemuDomainDetachControllerDevice will detach > the controller for sure and qemuDomainRemoveHostDevice removes a > hostdev, but I'm not seeing the connection. What QEMU does is well > perhaps a different story! Digging back through my notes... You're right, this is a QEMU thing that I can recreate without libvirt. So, bad wording in the commit message on my part.

> > >> It appears that something is tied up in the host virtio layer. > But that wouldn't be the case with the patch, true? Not a very friendly > thing to do I suspect - remove a controller whilst some hostdev is still > using it... Yup, very mean thing to do. > >> While I investigate this, we can see if a controller is being used by >> any hostdevs, and prevent the detach if it would lead us down this path. > Shall I assume the following is your "disk" example? Correct. To tie the above XML into the files used below: # cat scsicontroller.xml <controller type='scsi' model='virtio-scsi' index='0'/> # cat scsidisk.xml <hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='8' unit='1074151456'/> </source> </hostdev> (I probably should've named the latter "scsihostdev.xml" but it's at least closer than some other scratch files I have.) > >> $ virsh detach-device guest_one_virtio_scsi scsicontroller.xml >> error: Failed to detach device from scsicontroller.xml >> error: operation failed: device cannot be detached: device is busy >> >> $ virsh detach-device guest_one_virtio_scsi scsidisk.xml >> Device detached successfully >> >> $ virsh detach-device guest_one_virtio_scsi scsicontroller.xml >> Device detached successfully >> >> Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; >> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; >> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; >> --- >> src/qemu/qemu_hotplug.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index b03e618..5db7abf 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -4443,6 +4443,7 @@ static bool >> qemuDomainDiskControllerIsBusy(virDomainObjPtr vm, >> { >> size_t i; >> virDomainDiskDefPtr disk; >> + virDomainHostdevDefPtr hostdev; >> for (i = 0; i < vm->def->ndisks; i++) { >> disk = vm->def->disks[i]; >> @@ -4465,6 +4466,15 @@ static bool >> qemuDomainDiskControllerIsBusy(virDomainObjPtr vm, >> return true; >> } >> + for (i = 0; i < vm->def->nhostdevs; i++) { >> + hostdev = vm->def->hostdevs[i]; >> + if (!virHostdevIsSCSIDevice(hostdev) || >> + detach->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) >> + continue; >> + if (hostdev->info->addr.drive.controller == detach->idx) >> + return true; >> + } >> + > I certainly believe this should be added considering, but I want to make > sure I'm following the reasoning and order you've done the detach! > > Essentially you're trying to make sure there's no hostdev using the > controller before allowing it to be removed. Yup, that's it in a nutshell. > > I guess I'm just looking to clear up a few things in the commit message > before pushing... > > In particular, the bug being that libvirt will check *disks* using the > controller before allowing it to be detached/removed, but it doesn't > check *hostdevs* using the controller before allowing a controller > detach/remove to proceed. Yup, exactly. The patch I sent obviously focuses on SCSI hostdevs, because of the problem the guest then experiences. I didn't give any consideration to PCI or USB hostdev's, and whether they should be protected from a similarly lazy/rude detach. I honestly don't even know if the problem would exist in that configuration, or if it's tied up in virtio-scsi. - Eric > > Thanks and great catch, > > John >> return false; >> } >>