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