[libvirt] [PATCH 10/25] qemu: Introduce activeScsiHostdevs list for scsi host devices
Osier Yang
jyang at redhat.com
Tue May 7 11:27:50 UTC 2013
On 07/05/13 18:57, John Ferlan wrote:
> 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?)
Yes, I should make it more clear.
>
> 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."
Okay.
>> 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)...
Personally I like "!", but I don't have a good reason to keep it,
and keep consistence sounds good.
> 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;
Another personal habit I think, though I prefer to put them together
too.
>
>> 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.
The VIR_WARN is useless, as virSCSIDeviceNew has errors.
>
>> +
>> + 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?
It's not *taken* from list, I.E. list still keep the pointers, so they
are just
stealed (let's say detached) from activeList, and virObjectUnref will
takes care of freeing them.
That's why I don't like the vir{pci,usb}.[ch], it causes a lot of mess.
>
>> + 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?
virObjectUnref will do it, virSCSIDeviceListDispose is involked.
Osier
More information about the libvir-list
mailing list