Re: [libvirt] [PATCH] qemu: Prevent detaching SCSI controller used by 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.) > 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; >> } >>