[libvirt] [PATCH] qemu: Prevent detaching SCSI controller used by hostdev

[John Ferlan]

[...]

>>> 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 at linux.vnet.ibm.com>
>>> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>>> ---
>>>   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;
>>>   }
>>>  
>