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

[John Ferlan]

On 01/02/2014 09:45 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).
> 
> Also don't try to add the device to the activeScsiHostdevs list if
> it's already there.
> ---
>  src/qemu/qemu_hostdev.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 86a463a..8536499 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -1120,22 +1120,25 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
>          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 (!(virSCSIDeviceGetShareable(scsi) &&
> +                  virSCSIDeviceGetShareable(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;
> +            }
> +        } else {
> +            virSCSIDeviceSetUsedBy(scsi, name);
> +            VIR_DEBUG("Adding %s to activeScsiHostdevs", virSCSIDeviceGetName(scsi));
>  
> -        if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
> -            goto error;
> +            if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
> +                goto error;
> +        }


Something doesn't feel right here...

Prior code:

if find device on some other drivers active list
    fail
set name of who device is used by
add device to host active list
  (NOTE: will error if found on list already)


New code

if find device on some other drivers active list
    if both devices not marked shareable
        fail
else
    set name of who device is used by
    add device to host active list


So now theoretically with a shareable device, the device could be on the
active list and it's OK to use it because it's marked shareable.

So my question/issue becomes what to do with the "dev->used_by" (eg,
virSCSIDeviceSetUsedBy() result) value.  In fact, since we're now
shareable it seems to me that we'd need to keep this up to date even
going so far as to clear usage it when virSCSIDeviceListDel() is called.
Should the list become a "list" of those using it?  The first domain
that claims usage could eventually remove their usage, then what? It
would be strange to report domain X has the device in use if the domain
is down or doesn't exist.  Use cscope and check the callers/users of
"used_by" to see what my concern is.

Remember prior to this point in time we cannot share, so it didn't
matter. However, from this point on since we can share we need to keep
track of who has it, right?  What becomes even more interesting perhaps
is the impact on migration (if that's even possible with a shared hostdev).

John

>      }
>  
>      virObjectUnlock(driver->activeScsiHostdevs);
>