[...]
>>> 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.
>
Given all this - can you just provide an updated commit message. I can
replace before pushing before the current release.
Tks -
John
>>
>>
>>> 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@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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;
>>> }
>>>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
