Re: [libvirt RFC] qemu_saveimage: only use wrapper when using --bypass-cache
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(a)suse.de>
> ---
> 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(a)redhat.com>
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