[libvirt] [PATCH] qemu: Introduce a wrapper over virFileWrapperFdClose

[Michal Privoznik]

On 09/18/2017 10:49 PM, John Ferlan wrote:
> 
> 
> On 09/18/2017 05:08 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1448268
>>
>> When migrating to a file (e.g. when doing 'virsh save file'),
>> couple of things are happening in the thread that is executing
>> the API:
>>
>> 1) the domain obj is locked
>> 2) iohelper is spawned as a separate process to handle all I/O
>> 3) the thread waits for iohelper to finish
>> 4) the domain obj is unlocked
>>
>> Now, the problem is that while the thread waits in step 3 for
>> iohelper to finish this may take ages because iohelper calls
>> fdatasync(). And unfortunately, we are waiting the whole time
>> with the domain locked. So if another thread wants to jump in and
>> say copy the domain name ('virsh list' for instance), they are
>> stuck.
>>
>> The solution is to unlock the domain whenever waiting for I/O and
>> lock it back again when it finished.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/qemu/qemu_driver.c | 27 +++++++++++++++++++++++----
>>  1 file changed, 23 insertions(+), 4 deletions(-)
>>
> 
> I would think there'd be more concern about dropping the lock in the
> middle and something changing that could more negatively affect things.
> There's such a delicate balance between Save/Restore processing now - if
> you adjust the locks that could mean simultaneous usage, right?

Yes & no. Yes in sense that if another thread (in this case the one that
is listing the domains) wants to access the domain it can. But no other
API can be started really (like migrate, or hotplug) because the thread
executing save/dump sets a job. A job is like a flag so that we can
unlock the domain object for others to use (in a limited manner) and yet
still exclude another state changing API. We lock and unlock domains all
the time btw - qemuDomaninObjEnterMonitor() and
qemuDomainObjExitMonitor() do just that. And exactly for the same reason
as here. It may take ages for qemu to reply to our command and for that
the domain obj lock should not be held. IOW, once a job is acquired we
can lock & unlock domain as we please because we can be sure nobody will
change the libvirt internal state of the domain. All that other threads
can do is read.

> 
> Perhaps thinking differently... Does the virdomainobjlist code for
> Collect, Filter, Export really need to obtain the domain obj write lock?

Unfortunately yes. vm->def is not immutable, nor vm->def->name.

> 
> If Export is but a "snapshot in time", then using virObject{Lock|Unlock}
> perhaps isn't necessary in any of the @vms list processing. Since
> virDomainObjListCollectIterator sets a refcnt on the object it cannot be
> freed. Beyond that any state checking done in virDomainObjListFilter is
> negated because the lock isn't held until the virGetDomain call is made
> during virDomainObjListExport to fill in the returned @doms list. It's
> quite possible after *Filter to have the 'removing' boolean get set
> before the virGetDomain call is made. So what's the need or purpose of
> the write locks then?
> 
> If the object locks are removed during Export, then virsh list won't
> freeze while other operations that take time occur. Since the RWLock is
> taken on the list while @vms is created, we know nothing else can
> Add/Remove to/from the list - thus *that* is our snapshot in time.
> Anything done after than is just reducing what's in that snapshot.
> 
> It's "interesting" that the domain export list uses a very different
> model than other drivers... That loop to take a refcnt on each object w/
> the Read lock taken is something I could see duplicated for other
> drivers conceptually speaking of course if someone were to write that
> kind of common processing model ;-)

Right. It's not domain obj lock that is RW, it's the domain obj *list*
lock that is. The domain obj lock is still a mutex.

BTW: it's not only 'virsh list' that would hang. Other APIs that just
read without grabbing a job would hang too:

qemuDomainIsActive
qemuDomainIsPersistent
..
And also qemuDomainLookupByName.

We really must unlock the domain instead of trying to work around the
problem.

Michal