Re: [PATCH v6 09/10] migration/yank: Keep track of registered yank instances

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Sep 11, 2023 at 02:13:19PM -0300, Fabiano Rosas wrote:
>> The core yank code is strict about balanced registering and
>> unregistering of yank functions.
>> 
>> This creates a difficulty because the migration code registers one
>> yank function per QIOChannel, but each QIOChannel can be referenced by
>> more than one QEMUFile. The yank function should not be removed until
>> all QEMUFiles have been closed.
>> 
>> Keep a reference count of how many QEMUFiles are using a QIOChannel
>> that has a yank function. Only unregister the yank function when all
>> QEMUFiles have been closed.
>> 
>> This improves the current code by removing the need for the programmer
>> to know which QEMUFile is the last one to be cleaned up and fixes the
>> theoretical issue of removing the yank function while another QEMUFile
>> could still be using the ioc and require a yank.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/yank_functions.c | 81 ++++++++++++++++++++++++++++++++++----
>>  migration/yank_functions.h |  8 ++++
>>  2 files changed, 81 insertions(+), 8 deletions(-)
>
> I worry this over-complicate things.

It does. We ran out of simple options.

> If you prefer the cleaness that we operate always on qemufile level, can we
> just register each yank function per-qemufile?

"just" hehe

we could, but:

i) the yank is a per-channel operation, so this is even more unintuitive;

ii) multifd doesn't have a QEMUFile, so it will have to continue using
    the ioc;

iii) we'll have to add a yank to every new QEMUFile created during the
     incoming migration (colo, rdma, etc), otherwise the incoming side
     will be left using iocs while the src uses the QEMUFile;

iv) this is a functional change of the yank feature for which we have no
    tests.

If that's all ok to you I'll go ahead and git it a try.

> I think qmp yank will simply fail the 2nd call on the qemufile if the
> iochannel is shared with the other one, but that's totally fine, IMHO.
>
> What do you think?
>
> In all cases, we should probably at least merge patch 1-8 if that can
> resolve the CI issue.  I think all of them are properly reviewed.

I agree. Someone needs to queue this though since Juan has been busy.