Peter Xu <pet...@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 <faro...@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.
