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

[Osier Yang]

On 16/01/14 23:39, John Ferlan wrote:
>
> On 01/16/2014 08:53 AM, Osier Yang wrote:
>
> <...snip...>
>>> 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.
>>>
>> Missed reply for this one.
>>
>> Actually it needs the checking.  Assuming there are 2 live domains, and
>> they are using the same SCSI generic device.  It's possible since the
>> device could be shared among domains.   And in that case, we should
>> update the "dev->used_by" array instead of adding the device to the list
>> ("driver->activeScsiHostdevs") directly, during the reconnecting sequence.
>> I.E it needs to restore the internal data correctly to the state just as the
>> one before libvirtd getting shutdown.
>>
> The activeScsiHostdevs is listed off the driver and not the domain,

Yes. It's of the driver.

> so
> it's not like each domain has to update which other domains is sharing.

But the list is filled while each domain is starting or *reconnecting*

> Maybe I'm misreading your thoughts regarding needing to update instead
> of add.
>
> If there were 2..n already running with the share attribute already set,
> would the reconnect threads run/finish before the new libvirtd accepts
> new connections/domain starts?

Yes.  See daemonRunStateInit.

> If so, then this code will restore life
> as it was before libvirtd was stopped.

Yes. That's what this patch tries to do.

> The only way those domains would
> have been able to start previously and share the device is if they ran
> through the qemuPrepareHostdevSCSIDevices() which does check shareable.

Yes, that's why I didn't do the checking about "shareable" in
qemuUpdateActivePciHostdevs.

>
> My assumption was after some amount of code reading - those restart
> threads would finish so that it's not possible for some domain to be
> started that isn't sharing, but wanting to use the same device.

Exactly right.

>
> Since prior code doesn't allow sharing of these devices there doesn't
> need to be a consideration made for an already running domain when this
> code first appears. Although, you may run into situations where
> qemuPrepareHostdevSCSIDevices() fails due to a running domain that was
> running before the shareable attribute was known.

Indeed, but we have no way to avoid this problem for the domains
which was running against the old libvirt, and the libvirt is upgraded
after that.

> That domain would have
> to be restarted - something that could be documentable. The error
> message could be more helpful indicating it's in use without the
> shareable attribute by some other domain.

It's more complicate than this, when a domain is trying to start,
the  situations could be (with new code):

1) No domain is using the device

2)  1 domain is using the device

3) More than 1 domain are using the device.

For 1), it's just fine, let's ignore it.

For 2), the domain which is using it could use the device as either
"shareable" or "non-shareable".  If it's "shareable", the later domain
could be started successfully only if it uses the "device" as "shareable"
too;  If it's "non-shareable", the later domain can't use the device anyway.

For 3), the domains must use the device as "shareable". And the later
domain must use the device as "shareable" too to be started successfully.

So, as you seen, we can't simply say with error "the device is in use
without shareable attribute by some other domain". We can use if...else
branches to construct the more sensible errors, but I'm doubting about
if we should make things more complicate.

Though for old code, simply saying "the device is in use without
the shareable attribute by some other domain" is fine, but if you think
about the situations in new code, it will be a mess.

>   In that case, only 1 domain
> would be using it thus in theory used_by[0] would be that domain. If
> there were more than 1 domain (n_used_by), then you've got other
> problems in the code!

I guess here you still mean the domain(s) which were running with
the old code.  And since with the old code, there could be only 1
domain using the same device, regardless of the device is specified
as "shareable" or not. So yet, the "n_used_by" must be 1.

As a conclusion, I think the only concern from you is about the problem
on the running domain of an old libvirt (without these 2 patches). Right?
If so, my thought is to add document somewhere, though I have not
much idea about where to put the document, and how to write the
document to explain the problem which is such complicated.

Osier