[libvirt] [PATCH] qemu: Prevent detaching SCSI controller used by hostdev
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'/> </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.) Detaching the hostdev, followed by the controller, works well and the guest behaves appropriately. 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. It appears that something is tied up in the host virtio layer. 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. $ 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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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; + } + return false; } -- 2.8.4
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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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; }
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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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; }
[...]
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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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; }
Consider the following XML snippets: $ cat scsicontroller.xml <controller type='scsi' model='virtio-scsi' index='0'/> $ cat scsihostdev.xml <hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='8' unit='1074151456'/> </source> </hostdev> If we create a guest that includes the contents of scsihostdev.xml, but forget the virtio-scsi controller described in scsicontroller.xml, one is silently created for us. The same holds true when attaching a hostdev before the matching virtio-scsi controller. (See qemuDomainFindOrCreateSCSIDiskController for context.) Detaching the hostdev, followed by the controller, works well and the guest behaves appropriately. If we detach the virtio-scsi controller device first, any associated hostdevs are detached for us by the underlying virtio-scsi code (this is fine, since the connection is broken). 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. While this is not libvirt's problem, we can prevent falling into this scenario by checking if a controller is being used by any hostdev devices. The same is already done for disk elements today. Applying this patch and then using the XML snippets from earlier: $ virsh detach-device guest_01 scsicontroller.xml error: Failed to detach device from scsicontroller.xml error: operation failed: device cannot be detached: device is busy $ virsh detach-device guest_01 scsihostdev.xml Device detached successfully $ virsh detach-device guest_01 scsicontroller.xml Device detached successfully Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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; + } + return false; } -- 2.8.4
On 11/29/2016 04:40 PM, Eric Farman wrote:
Consider the following XML snippets:
$ cat scsicontroller.xml <controller type='scsi' model='virtio-scsi' index='0'/> $ cat scsihostdev.xml <hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='8' unit='1074151456'/> </source> </hostdev>
If we create a guest that includes the contents of scsihostdev.xml, but forget the virtio-scsi controller described in scsicontroller.xml, one is silently created for us. The same holds true when attaching a hostdev before the matching virtio-scsi controller. (See qemuDomainFindOrCreateSCSIDiskController for context.)
Detaching the hostdev, followed by the controller, works well and the guest behaves appropriately.
If we detach the virtio-scsi controller device first, any associated hostdevs are detached for us by the underlying virtio-scsi code (this is fine, since the connection is broken). 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.
While this is not libvirt's problem, we can prevent falling into this scenario by checking if a controller is being used by any hostdev devices. The same is already done for disk elements today.
Applying this patch and then using the XML snippets from earlier:
$ virsh detach-device guest_01 scsicontroller.xml error: Failed to detach device from scsicontroller.xml error: operation failed: device cannot be detached: device is busy
$ virsh detach-device guest_01 scsihostdev.xml Device detached successfully
$ virsh detach-device guest_01 scsicontroller.xml Device detached successfully
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/qemu/qemu_hotplug.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
ACK, safe, and pushed. Thanks, John
participants (2)
-
Eric Farman -
John Ferlan