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

[Osier Yang]

On 06/01/14 23:54, John Ferlan wrote:
>
> 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.

Yes, as long as the later domain(s) specify the device as shareable
too.

>
> 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?

/Alarm....

Yes, thanks for pointing it out...

> 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.

It actually won't report error. Since the "dev->used_by" is freed
along with the domain destroying. E.g.

% start domain A with sg7 as shareable.

% start domain B with sg7 as shareable

% Destroy domain A

% start domain C with sg7 as non-shareable.

Starting Domain C is expected to fail, but it will succeed.

I will post the v2.

>   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).

It should be fine for migration, as long as we make the "used_by"
correct.

Regards,
Osier