[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains




On 01/08/2014 09:51 AM, Osier Yang wrote:
> It doesn't make sense to fail if the SCSI host device is specified
> as "shareable" explicitly between domains (NB, it works if and only
> if the device is specified as "shareable" for *all* domains,
> otherwise it fails).
> 
> To fix the problem, this patch introduces an array for virSCSIDevice
> struct, which records all the names of domain which are using the
> device (note that the recorded domains must specifiy the device as
> shareable).  And the change on the data struct brings on many
> subsequent changes in the code.
> 
> * src/util/virscsi.h:
>   - Remove virSCSIDeviceGetUsedBy
>   - Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel
>   - Add virSCSIDeviceIsAvailable
> 
> * src/util/virscsi.c:
>   - struct virSCSIDevice: Change "used_by" to be an array; Add
>     "n_used_by" as the array count
>   - virSCSIDeviceGetUsedBy: Removed
>   - virSCSIDeviceFree: frees the "used_by" array
>   - virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential
>     memory corruption
>   - virSCSIDeviceIsAvailable: New
>   - virSCSIDeviceListDel: Change the logic, for device which is already
>     in the list, just remove the corresponding entry in "used_by"
>   - Copyright updating
> 
> * src/libvirt_private.sys:
>   - virSCSIDeviceGetUsedBy: Remove
>   - virSCSIDeviceIsAvailable: New
> 
> * src/qemu/qemu_hostdev.c:
>   - qemuUpdateActiveScsiHostdevs: Check if the device existing before
>     adding it to the list;
>   - qemuPrepareHostdevSCSIDevices: Error out if the not all domains
>     use the device as "shareable"; Also don't try to add the device
>     to the activeScsiHostdevs list if it already there.
>   - qemuDomainReAttachHostScsiDevices: Change the logic according
>     to the changes on helpers.
> ---
>  src/libvirt_private.syms |  2 +-
>  src/qemu/qemu_hostdev.c  | 75 ++++++++++++++++++++++++++----------------------
>  src/util/virscsi.c       | 48 +++++++++++++++++++++++++------
>  src/util/virscsi.h       |  7 +++--
>  4 files changed, 84 insertions(+), 48 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 65d1bde..bd5f466 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1677,7 +1677,7 @@ virSCSIDeviceGetSgName;
>  virSCSIDeviceGetShareable;
>  virSCSIDeviceGetTarget;
>  virSCSIDeviceGetUnit;
> -virSCSIDeviceGetUsedBy;
> +virSCSIDeviceIsAvailable;
>  virSCSIDeviceListAdd;
>  virSCSIDeviceListCount;
>  virSCSIDeviceListDel;
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 86a463a..9d81e94 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -250,13 +250,14 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
>      virDomainHostdevDefPtr hostdev = NULL;
>      size_t i;
>      int ret = -1;
> +    virSCSIDevicePtr scsi = NULL;
> +    virSCSIDevicePtr tmp = NULL;
>  
>      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 ||
> @@ -271,11 +272,18 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
>                                        hostdev->shareable)))
>              goto cleanup;
>  
> -        virSCSIDeviceSetUsedBy(scsi, def->name);
> -
> -        if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
> +        if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
> +            if (virSCSIDeviceSetUsedBy(tmp, def->name) < 0) {
> +                virSCSIDeviceFree(scsi);
> +                goto cleanup;
> +            }
>              virSCSIDeviceFree(scsi);
> -            goto cleanup;
> +        } else {
> +            if (virSCSIDeviceSetUsedBy(scsi, def->name) < 0 ||
> +                virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
> +                virSCSIDeviceFree(scsi);
> +                goto cleanup;
> +            }

It took some thinking, but I convinced myself that this path doesn't
need the shared check since it's only called from qemuProcessReconnect;
however, if something else did call it some day then that check may be
necessary. It may be wise to add it anyway... I have no strong opinion
about it being required for this change.

>          }
>      }
>      ret = 0;
> @@ -1118,24 +1126,26 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
>      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
> +            if (!(virSCSIDeviceGetShareable(scsi) &&
> +                  virSCSIDeviceGetShareable(tmp))) {
>                  virReportError(VIR_ERR_OPERATION_INVALID,
> -                               _("SCSI device %s is already in use"),
> +                               _("SCSI device %s is already in use "
> +                                 "by other domain(s)"),
>                                 virSCSIDeviceGetName(tmp));
> -            goto error;
> -        }
> +                goto error;
> +            }
>  
> -        virSCSIDeviceSetUsedBy(scsi, name);
> -        VIR_DEBUG("Adding %s to activeScsiHostdevs", virSCSIDeviceGetName(scsi));
> +            if (virSCSIDeviceSetUsedBy(tmp, name) < 0)
> +                goto error;
> +        } else {
> +            if (virSCSIDeviceSetUsedBy(scsi, name) < 0)
> +                goto error;
>  
> -        if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
> -            goto error;
> +            VIR_DEBUG("Adding %s to activeScsiHostdevs", virSCSIDeviceGetName(scsi));
> +
> +            if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
> +                goto error;
> +        }
>      }
>  
>      virObjectUnlock(driver->activeScsiHostdevs);
> @@ -1380,8 +1390,7 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
>      virObjectLock(driver->activeScsiHostdevs);
>      for (i = 0; i < nhostdevs; i++) {
>          virDomainHostdevDefPtr hostdev = hostdevs[i];
> -        virSCSIDevicePtr scsi, tmp;
> -        const char *used_by = NULL;
> +        virSCSIDevicePtr scsi;
>          virDomainDeviceDef dev;
>  
>          dev.type = VIR_DOMAIN_DEVICE_HOSTDEV;
> @@ -1411,30 +1420,26 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
>          /* 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) {
> +        if (!virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi)) {

I think you should keep (and perhaps rename) the tmp pointer and then
pass it to virSCSIDeviceListDel() since that function will call the the
find again anyway

>              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);
> +            virSCSIDeviceFree(scsi);
>              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);
> +        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);
> -        }
> +        virSCSIDeviceListDel(driver->activeScsiHostdevs, scsi, name);
> +        virSCSIDeviceFree(scsi);
>      }
>      virObjectUnlock(driver->activeScsiHostdevs);
>  }
> diff --git a/src/util/virscsi.c b/src/util/virscsi.c
> index 3998c3a..42030c5 100644
> --- a/src/util/virscsi.c
> +++ b/src/util/virscsi.c
> @@ -1,6 +1,7 @@
>  /*
>   * virscsi.c: helper APIs for managing host SCSI devices
>   *
> + * Copyright (C) 2013 - 2014 Red Hat, Inc.
>   * Copyright (C) 2013 Fujitsu, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -55,7 +56,8 @@ struct _virSCSIDevice {
>      char *name; /* adapter:bus:target:unit */
>      char *id;   /* model:vendor */
>      char *sg_path; /* e.g. /dev/sg2 */
> -    const char *used_by; /* name of the domain using this dev */
> +    char **used_by; /* name of the domains using this dev */
> +    size_t n_used_by; /* how many domains are using this dev */
>  
>      bool readonly;
>      bool shareable;
> @@ -256,26 +258,37 @@ cleanup:
>  void
>  virSCSIDeviceFree(virSCSIDevicePtr dev)
>  {
> +    size_t i;
> +
>      if (!dev)
>          return;
>  
>      VIR_FREE(dev->id);
>      VIR_FREE(dev->name);
>      VIR_FREE(dev->sg_path);
> +    for (i = 0; i < dev->n_used_by; i++) {
> +        VIR_FREE(dev->used_by[i]);
> +    }
> +    VIR_FREE(dev->used_by);
>      VIR_FREE(dev);
>  }
>  
> -void
> +int
>  virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev,
>                         const char *name)
>  {
> -    dev->used_by = name;
> +    char *copy = NULL;
> +
> +    if (VIR_STRDUP(copy, name) < 0)
> +        return -1;
> +
> +    return VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy);
>  }
>  
> -const char *
> -virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev)
> +bool
> +virSCSIDeviceIsAvailable(virSCSIDevicePtr dev)
>  {
> -    return dev->used_by;
> +    return dev->n_used_by == 0;
>  }
>  
>  const char *
> @@ -406,10 +419,27 @@ virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
>  
>  void
>  virSCSIDeviceListDel(virSCSIDeviceListPtr list,
> -                     virSCSIDevicePtr dev)
> +                     virSCSIDevicePtr dev,
> +                     const char *name)
>  {
> -    virSCSIDevicePtr ret = virSCSIDeviceListSteal(list, dev);
> -    virSCSIDeviceFree(ret);
> +    virSCSIDevicePtr ret = NULL;
> +    size_t i;
> +
> +    if (!(ret = virSCSIDeviceListFind(list, dev)))
> +        return;

since there's only one caller and it already did a
virSCSIDeviceListFind() (but threw away the results) - couldn't the
caller save and pass the results rather than making the same call twice?

I don't think a V3 would be necessary based on your thoughts.  Again -
this is something for post 1.2.1

John
> +
> +    for (i = 0; i < ret->n_used_by; i++) {
> +        if (STREQ_NULLABLE(ret->used_by[i], name)) {
> +            if (ret->n_used_by > 1) {
> +                VIR_DELETE_ELEMENT(ret->used_by, i, ret->n_used_by);
> +            } else {
> +                ret = virSCSIDeviceListSteal(list, dev);
> +                virSCSIDeviceFree(ret);
> +            }
> +
> +            break;
> +        }
> +    }
>  }
>  
>  virSCSIDevicePtr
> diff --git a/src/util/virscsi.h b/src/util/virscsi.h
> index b2e98ca..aff7e5a 100644
> --- a/src/util/virscsi.h
> +++ b/src/util/virscsi.h
> @@ -50,8 +50,8 @@ virSCSIDevicePtr virSCSIDeviceNew(const char *adapter,
>                                    bool shareable);
>  
>  void virSCSIDeviceFree(virSCSIDevicePtr dev);
> -void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
> -const char *virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev);
> +int virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
> +bool virSCSIDeviceIsAvailable(virSCSIDevicePtr dev);
>  const char *virSCSIDeviceGetName(virSCSIDevicePtr dev);
>  unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev);
>  unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev);
> @@ -83,7 +83,8 @@ size_t virSCSIDeviceListCount(virSCSIDeviceListPtr list);
>  virSCSIDevicePtr virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
>                                          virSCSIDevicePtr dev);
>  void virSCSIDeviceListDel(virSCSIDeviceListPtr list,
> -                          virSCSIDevicePtr dev);
> +                          virSCSIDevicePtr dev,
> +                          const char *name);
>  virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list,
>                                         virSCSIDevicePtr dev);
>  
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]