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

[Eric Farman]

On 11/28/2016 04:14 PM, John Ferlan wrote:
>
> On 11/28/2016 09:01 AM, Eric Farman wrote:
>> 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'/>
> That's an awfully large unit # isn't it?  For the guest...

Large, yes.  Unreasonable, no.  This is the address on the host, which 
is connected to a rather large box of SCSI disks.  Everything is in the 
neighborhood of 0x40xx40yy00000000:

# lsscsi | grep sdg
[0:0:8:1074151456]disk    IBM      2107900          .217  /dev/sdg
# lsscsi -xx | grep sdg
[0:0:8:0x4020400600000000] disk    IBM      2107900          .217 /dev/sdg

In the guest, I'm placing it at address 0:0:0:0, just didn't specify one 
in the example.


>
>>        </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.)
> 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 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;
>>   }
>>   
>>