[libvirt] [PATCH 10/25] qemu: Introduce activeScsiHostdevs list for scsi host devices
John Ferlan
jferlan at redhat.com
Tue May 7 10:57:37 UTC 2013
On 05/03/2013 02:07 PM, Osier Yang wrote:
> From: Han Cheng <hanc.fnst at cn.fujitsu.com>
>
> Although virtio-scsi supports SCSI PR, the device on host may do not
> support it. To avoid losing data, we only allow one scsi hostdev to
> be passthroughed to one live guest, Just like what we do for PCI
> and USB devices.
What is "PR"? (Persistent Reservations?)
s/To avoid losing data....USB devices/
"Just like PCI and USB pass through devices, only one live guest is
allowed per SCSI host pass through device."
>
> Signed-off-by: Han Cheng <hanc.fnst at cn.fujitsu.com>
> ---
> src/qemu/qemu_conf.h | 2 +
> src/qemu/qemu_driver.c | 3 +
> src/qemu/qemu_hostdev.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_hostdev.h | 10 +++
> src/qemu/qemu_process.c | 3 +
> 5 files changed, 231 insertions(+)
>
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 77d3d2f..30a2a22 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -36,6 +36,7 @@
> # include "security/security_manager.h"
> # include "virpci.h"
> # include "virusb.h"
> +# include "virscsi.h"
> # include "cpu_conf.h"
> # include "driver.h"
> # include "virportallocator.h"
> @@ -203,6 +204,7 @@ struct _virQEMUDriver {
> virPCIDeviceListPtr activePciHostdevs;
> virPCIDeviceListPtr inactivePciHostdevs;
> virUSBDeviceListPtr activeUsbHostdevs;
> + virSCSIDeviceListPtr activeScsiHostdevs;
>
> /* Immutable pointer. Unsafe APIs. XXX */
> virHashTablePtr sharedDisks;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d5d7de3..b631a1f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -675,6 +675,9 @@ qemuStateInitialize(bool privileged,
> if ((qemu_driver->inactivePciHostdevs = virPCIDeviceListNew()) == NULL)
> goto error;
>
> + if (!(qemu_driver->activeScsiHostdevs = virSCSIDeviceListNew()))
> + goto error;
> +
A nit, but keeping things with the same look and feel in code is
sometimes easier rather than something that looks different, but
accomplishes the same thing. So rather than !(), use the existing () ==
NULL)...
Furthermore a bunch of those if's could probably be put together, but
that's outside of this set of changes... eg, if (cond1 || cond2 ||
cond3 || etc) goto error;
> if (!(qemu_driver->sharedDisks = virHashCreate(30, qemuSharedDiskEntryFree)))
> goto error;
>
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index e4af036..d5f94d5 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -29,6 +29,7 @@
> #include "viralloc.h"
> #include "virpci.h"
> #include "virusb.h"
> +#include "virscsi.h"
> #include "virnetdev.h"
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
> @@ -226,6 +227,47 @@ cleanup:
> return ret;
> }
>
> +int
> +qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
> + virDomainDefPtr def)
> +{
> + virDomainHostdevDefPtr hostdev = NULL;
> + int i;
> + int ret = -1;
> +
> + if (!def->nhostdevs)
> + return 0;
> +
> + virObjectLock(driver->activeScsiHostdevs);
> + for (i = 0; i < def->nhostdevs; i++) {
> + virSCSIDevicePtr scsi = NULL;
> + hostdev = def->hostdevs[i];
> +
> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
> + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
> + continue;
> +
> + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit,
> + hostdev->readonly)))
> + goto cleanup;
I see that the qemuUpdateActiveUsbHostdevs() had a VIR_WARN before the
goto cleanup - something you may want to consider here as well.
> +
> + virSCSIDeviceSetUsedBy(scsi, def->name);
> +
> + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
> + virSCSIDeviceFree(scsi);
> + goto cleanup;
> + }
> + }
> + ret = 0;
> +
> +cleanup:
> + virObjectUnlock(driver->activeScsiHostdevs);
> + return ret;
> +}
> +
> static int
> qemuDomainHostdevPciSysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path)
> {
> @@ -827,6 +869,107 @@ cleanup:
> return ret;
> }
>
> +int
> +qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
> + const char *name,
> + virDomainHostdevDefPtr *hostdevs,
> + int nhostdevs)
> +{
> + int i, j, count;
> + virSCSIDeviceListPtr list;
> + virSCSIDevicePtr tmp;
> +
> + /* To prevent situation where SCSI device is assigned to two domains
> + * we need to keep a list of currently assigned SCSI devices.
> + * This is done in several loops which cannot be joined into one big
> + * loop. See qemuPrepareHostdevPCIDevices()
> + */
> + if (!(list = virSCSIDeviceListNew()))
> + goto cleanup;
> +
> + /* Loop 1: build temporary list */
And I see from PrepareHostDevices that nhostdevs is guaranteed to be non
zero.... good...
> + for (i = 0 ; i < nhostdevs ; i++) {
> + virDomainHostdevDefPtr hostdev = hostdevs[i];
> + virSCSIDevicePtr scsi;
> +
> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
> + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
> + continue;
> +
> + if (hostdev->managed) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("SCSI host device doesn't support managed mode"));
> + goto cleanup;
> + }
> +
> + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit,
> + hostdev->readonly)))
> + goto cleanup;
> +
> + if (scsi && virSCSIDeviceListAdd(list, scsi) < 0) {
> + virSCSIDeviceFree(scsi);
> + goto cleanup;
> + }
> + }
> +
> + /* Loop 2: Mark devices in temporary list as used by @name
> + * and add them to driver list. However, if something goes
> + * wrong, perform rollback.
> + */
> + virObjectLock(driver->activeScsiHostdevs);
> + count = virSCSIDeviceListCount(list);
> +
> + for (i = 0; i < count; i++) {
> + virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i);
> + if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
> + const char *other_name = virSCSIDeviceGetUsedBy(tmp);
> +
> + if (other_name)
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("SCSI device %s is in use by domain %s"),
> + virSCSIDeviceGetName(tmp), other_name);
> + else
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("SCSI device %s is already in use"),
> + virSCSIDeviceGetName(tmp));
> + goto error;
> + }
> +
> + virSCSIDeviceSetUsedBy(scsi, name);
> + VIR_DEBUG("Adding %s to activeScsiHostdevs", virSCSIDeviceGetName(scsi));
> +
> + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
> + goto error;
> + }
> +
> + virObjectUnlock(driver->activeScsiHostdevs);
> +
> + /* Loop 3: Temporary list was successfully merged with
> + * driver list, so steal all items to avoid freeing them
> + * when freeing temporary list.
> + */
> + while (virSCSIDeviceListCount(list) > 0) {
> + tmp = virSCSIDeviceListGet(list, 0);
> + virSCSIDeviceListSteal(list, tmp);
> + }
> +
> + virObjectUnref(list);
> + return 0;
> +
> +error:
> + for (j = 0; j < i; j++) {
> + tmp = virSCSIDeviceListGet(list, i);
> + virSCSIDeviceListSteal(driver->activeScsiHostdevs, tmp);
> + }
So what happens to the devices that were taken from 'list', had a 'name'
associated with them on the active list? Don't they need to have the
name now disassociated? And be removed from there?
> + virObjectUnlock(driver->activeScsiHostdevs);
> +cleanup:
If we get here from loop1, since the 'list' is potentially full of
'scsi' devices via virSCSIDeviceListAdd(), should there be a loop to
remove those or does this Unref do that for you? Or is that what loop3
is supposed to do?
In general I think the error paths needs some cleanup/clarification
> + virObjectUnref(list);
> + return -1;
> +}
> +
> int qemuPrepareHostDevices(virQEMUDriverPtr driver,
> virDomainDefPtr def,
> bool coldBoot)
> @@ -841,6 +984,10 @@ int qemuPrepareHostDevices(virQEMUDriverPtr driver,
> if (qemuPrepareHostUSBDevices(driver, def, coldBoot) < 0)
> return -1;
>
> + if (qemuPrepareHostdevSCSIDevices(driver, def->name,
> + def->hostdevs, def->nhostdevs) < 0)
> + return -1;
> +
> return 0;
> }
>
> @@ -1025,6 +1172,69 @@ qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr driver,
> virObjectUnlock(driver->activeUsbHostdevs);
> }
>
> +void
> +qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
> + const char *name,
> + virDomainHostdevDefPtr *hostdevs,
> + int nhostdevs)
> +{
Follows the ReAttachHostUsbDevices() logic (other than the
hostdev->missing check).
John
> + int i;
> +
> + virObjectLock(driver->activeScsiHostdevs);
> + for (i = 0; i < nhostdevs; i++) {
> + virDomainHostdevDefPtr hostdev = hostdevs[i];
> + virSCSIDevicePtr scsi, tmp;
> + const char *used_by = NULL;
> +
> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
> + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
> + continue;
> +
> + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit,
> + hostdev->readonly))) {
> + VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain %s",
> + hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit,
> + name);
> + continue;
> + }
> +
> + /* Only delete the devices which are marked as being used by @name,
> + * because qemuProcessStart could fail on the half way. */
> +
> + tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi);
> + virSCSIDeviceFree(scsi);
> +
> + if (!tmp) {
> + VIR_WARN("Unable to find device %s:%d:%d:%d "
> + "in list of active SCSI devices",
> + hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit);
> + continue;
> + }
> +
> + used_by = virSCSIDeviceGetUsedBy(tmp);
> + if (STREQ_NULLABLE(used_by, name)) {
> + VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs",
> + hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit,
> + name);
> +
> + virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp);
> + }
> + }
> + virObjectUnlock(driver->activeScsiHostdevs);
> +}
> +
> void qemuDomainReAttachHostDevices(virQEMUDriverPtr driver,
> virDomainDefPtr def)
> {
> @@ -1036,4 +1246,7 @@ void qemuDomainReAttachHostDevices(virQEMUDriverPtr driver,
>
> qemuDomainReAttachHostUsbDevices(driver, def->name, def->hostdevs,
> def->nhostdevs);
> +
> + qemuDomainReAttachHostScsiDevices(driver, def->name, def->hostdevs,
> + def->nhostdevs);
> }
> diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
> index a1b8b9e..327d4d5 100644
> --- a/src/qemu/qemu_hostdev.h
> +++ b/src/qemu/qemu_hostdev.h
> @@ -31,6 +31,8 @@ int qemuUpdateActivePciHostdevs(virQEMUDriverPtr driver,
> virDomainDefPtr def);
> int qemuUpdateActiveUsbHostdevs(virQEMUDriverPtr driver,
> virDomainDefPtr def);
> +int qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
> + virDomainDefPtr def);
> int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
> const char *name,
> const unsigned char *uuid,
> @@ -42,9 +44,17 @@ int qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
> int qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver,
> const char *name,
> virUSBDeviceListPtr list);
> +int qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
> + const char *name,
> + virDomainHostdevDefPtr *hostdevs,
> + int nhostdevs);
> int qemuPrepareHostDevices(virQEMUDriverPtr driver,
> virDomainDefPtr def,
> bool coldBoot);
> +void qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
> + const char *name,
> + virDomainHostdevDefPtr *hostdevs,
> + int nhostdevs);
> void qemuReattachPciDevice(virPCIDevicePtr dev, virQEMUDriverPtr driver);
> void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
> const char *name,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 3dd178c..ff5fc89 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2972,6 +2972,9 @@ qemuProcessReconnect(void *opaque)
> if (qemuUpdateActiveUsbHostdevs(driver, obj->def) < 0)
> goto error;
>
> + if (qemuUpdateActiveScsiHostdevs(driver, obj->def) < 0)
> + goto error;
> +
> if (qemuInitCgroup(driver, obj, false) < 0)
> goto error;
>
>
More information about the libvir-list
mailing list