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

[John Ferlan]

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...

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

> 
> 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!


> 
> 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...

> 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?

> 
>   $ 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.

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.

Thanks and great catch,

John
>      return false;
>  }
>  
>