On 4/21/22 6:27 PM, Daniel P. Berrangé wrote:
> On Tue, Apr 12, 2022 at 11:18:15AM +0200, Claudio Fontana wrote:
>> align the "save" with the "restore" code,
>> by only using the wrapper when using --bypass-cache.
>>
>> This avoids a copy, resulting in better performance.
>>
>> Signed-off-by: Claudio Fontana <cfontana@xxxxxxx>
>> ---
>> src/qemu/qemu_saveimage.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
>> index 4fd4c5cfcd..5ea1b2fbcc 100644
>> --- a/src/qemu/qemu_saveimage.c
>> +++ b/src/qemu/qemu_saveimage.c
>> @@ -289,8 +289,10 @@ qemuSaveImageCreate(virQEMUDriver *driver,
>> if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
>> goto cleanup;
>>
>> - if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
>> - goto cleanup;
>> + if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
>> + if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
>> + goto cleanup;
>> + }
>
> This effectively reverts:
>
> commit c4caab538effb57411ad787fedb61e80d557caae
> Author: Jiri Denemark <jdenemar@xxxxxxxxxx>
> Date: Wed Feb 8 14:08:54 2012 +0100
>
> qemu: Always use iohelper for domain save
>
> This is probably not strictly needed as save operation is not live but
> we may have other reasons to avoid blocking qemu's main loop.
>
> As the commit message mentions, using a plain file FD results
> in the QEMU thread being blocked, even when O_NONBLOCK is used.
>
> Originally this impacted the main QEMU event loop, which means
> things like timers won't fire, or VNC clients won't be serviced,
> and the monitor won't respond while the save is taking place.
>
> Today IIUC, the migrate should run in a background thread, so
> much of these ill effects go away, as only the migration thread
> gets blocked.
>
> This would however impact the ability to cancel the migration
> operation. The cancel request could be delayed significantly
> if there's alot of pending disk I/O, as the migrate thread
> will be in kernel space in an uninterruptable sleep.
>
> Similarly if there is a problem with the host storage,
> eg dead NFS server, we'll be unable to cancel migration at
> all on the QEMU side.
>
> With the I/O helper, we'll still be unable to kill the I/O
> helper for the same reasons, but at least the impact is
> isolated from QEMU.
>
> I didn't realize that we actually passed the file FD to
> QEMU directly when restoring - I thought we always used
> the I/O helper there too. IMHO this is a bug in libvirt,
I think there might be more behind that.
I think it would be overkill to always use the wrapper, because of how qemuSaveImageOpen() has been implemented,
as opposed to qemuSaveImageCreate().
qemuSaveImageOpen is used for a plethora of use cases, and this stems I think from the fact that
sometimes libvirt just wants to operate on the XML it stores mixed up with the QEMU VM,
and sometimes it wants to actually read the whole QEMU VM.
(I wonder if storing them in separate files would have been preferable)
So the mid-way that has been found there seems to be, only use the wrapper when opening with bypass-cache,
otherwise the iohelper would be employed each and every time libvirt wants to access the xml as well.
Of course I cannot be sure of the intentions there.
Thanks,
Claudio
> as again QEMU has always (implicitly) expected a socket/pipe
> FD for fd: protocol with migration, and explicitly never
> tried to provide a 'file:' protocol.
>
> With regards,
> Daniel
>
