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@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;
+    }
 
     if (virQEMUSaveDataWrite(data, fd, path) < 0)
         goto cleanup;
-- 
2.34.1

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@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@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,
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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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@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@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
> 

On Thu, Apr 21, 2022 at 05:27:09PM +0100, 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@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@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.

Oh and the bit about 'save operation is not live' is actually
outdated. The VM CPUs are stopped when using 'virsh save'
but are not stopped when using 'virsh snapshot-create' for
live snapshots.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 4/21/22 6:52 PM, Daniel P. Berrangé wrote:
> On Thu, Apr 21, 2022 at 05:27:09PM +0100, 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@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@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.
> 

Then we are stuck with the extra copy apparently, if the helper cannot be taken out of the picture.

The problem is, the only way currently to get the maximum bandwidth out for a regular save (non-multifd), is to skip libvirt entirely,
which is very hard to maintain.

Maybe we need a --skip-iohelper option?


> Oh and the bit about 'save operation is not live' is actually
> outdated. The VM CPUs are stopped when using 'virsh save'
> but are not stopped when using 'virsh snapshot-create' for
> live snapshots.

So --skip-iohelper should be an option for only save and managedsave?

> 
> With regards,
> Daniel
> 

Thanks,

Claudio

On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@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;
> +    }

There is an obvious issue with this code. We are trying to close and
free a file descriptor that we have not opened when
VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.

>
>      if (virQEMUSaveDataWrite(data, fd, path) < 0)
>          goto cleanup;
> --
> 2.34.1
>

On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani@anisinha.ca> wrote:

> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@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;
> > +    }
>
> There is an obvious issue with this code. We are trying to close and
> free a file descriptor that we have not opened when
> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.


I meant *not* set in flags.


>
> >
> >      if (virQEMUSaveDataWrite(data, fd, path) < 0)
> >          goto cleanup;
> > --
> > 2.34.1
> >
>

On 4/12/22 7:23 PM, Ani Sinha wrote:
> On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani@anisinha.ca> wrote:
> 
>> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@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;
>>> +    }
>>
>> There is an obvious issue with this code. We are trying to close and
>> free a file descriptor that we have not opened when
>> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
> 
> 
> I meant *not* set in flags.

I don't think so. I don't think it is obvious, so can you be more specific?

From the function header comments it seems lecit and defined to call the function with NULL:

/**                                                                                                                     
 * virFileWrapperFdClose:                                                                                               
 * @wfd: fd wrapper, or NULL                                                                                            
 *                                                                                                                      
 * If @wfd is valid, then ensure that I/O has completed, which may                                                      
 * include reaping a child process.  Return 0 if all data for the                                                       
 * wrapped fd is complete, or -1 on failure with an error emitted.                                                      
 * This function intentionally returns 0 when @wfd is NULL, so that                                                     
 * callers can conditionally create a virFileWrapperFd wrapper but                                                      
 * unconditionally call the cleanup code.  To avoid deadlock, only                                                      
 * call this after closing the fd resulting from virFileWrapperFdNew().                                                 
 *                                                                                                                      
 * This function can be safely called multiple times on the same @wfd.                                                  
 */

Seems it has been specifically designed to simplify situations like this.

Thanks,

Claudio


> 
> 
>>
>>>
>>>      if (virQEMUSaveDataWrite(data, fd, path) < 0)
>>>          goto cleanup;
>>> --
>>> 2.34.1
>>>
>>
>

On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana <cfontana@suse.de> wrote:
>
> On 4/12/22 7:23 PM, Ani Sinha wrote:
> > On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani@anisinha.ca> wrote:
> >
> >> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@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;
> >>> +    }
> >>
> >> There is an obvious issue with this code. We are trying to close and
> >> free a file descriptor that we have not opened when
> >> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
> >
> >
> > I meant *not* set in flags.
>
> I don't think so. I don't think it is obvious, so can you be more specific?

See
   if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
        goto cleanup;

and under cleanup:
   if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
        ret = -1;

if VIR_DOMAIN_SAVE_BYPASS_CACHE not set in flags, both the above
function calls use wrapperFd initialized to NULL.
I think this patch would be incomplete if you do not handle all these scenarios.

>
> From the function header comments it seems lecit and defined to call the function with NULL:
>
> /**
>  * virFileWrapperFdClose:
>  * @wfd: fd wrapper, or NULL
>  *
>  * If @wfd is valid, then ensure that I/O has completed, which may
>  * include reaping a child process.  Return 0 if all data for the
>  * wrapped fd is complete, or -1 on failure with an error emitted.
>  * This function intentionally returns 0 when @wfd is NULL, so that
>  * callers can conditionally create a virFileWrapperFd wrapper but
>  * unconditionally call the cleanup code.  To avoid deadlock, only
>  * call this after closing the fd resulting from virFileWrapperFdNew().
>  *
>  * This function can be safely called multiple times on the same @wfd.
>  */
>
> Seems it has been specifically designed to simplify situations like this.
>
> Thanks,
>
> Claudio
>
>
> >
> >
> >>
> >>>
> >>>      if (virQEMUSaveDataWrite(data, fd, path) < 0)
> >>>          goto cleanup;
> >>> --
> >>> 2.34.1
> >>>
> >>
> >
>

On 4/13/22 6:13 AM, Ani Sinha wrote:
> On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana <cfontana@suse.de> wrote:
>>
>> On 4/12/22 7:23 PM, Ani Sinha wrote:
>>> On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani@anisinha.ca> wrote:
>>>
>>>> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@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;
>>>>> +    }
>>>>
>>>> There is an obvious issue with this code. We are trying to close and
>>>> free a file descriptor that we have not opened when
>>>> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
>>>
>>>
>>> I meant *not* set in flags.
>>
>> I don't think so. I don't think it is obvious, so can you be more specific?
> 
> See
>    if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
>         goto cleanup;
> 
> and under cleanup:
>    if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
>         ret = -1;
> 
> if VIR_DOMAIN_SAVE_BYPASS_CACHE not set in flags, both the above
> function calls use wrapperFd initialized to NULL.
> I think this patch would be incomplete if you do not handle all these scenarios.
> 
>>
>> From the function header comments it seems lecit and defined to call the function with NULL:
>>
>> /**
>>  * virFileWrapperFdClose:
>>  * @wfd: fd wrapper, or NULL
>>  *
>>  * If @wfd is valid, then ensure that I/O has completed, which may
>>  * include reaping a child process.  Return 0 if all data for the
>>  * wrapped fd is complete, or -1 on failure with an error emitted.
>>  * This function intentionally returns 0 when @wfd is NULL, so that
>>  * callers can conditionally create a virFileWrapperFd wrapper but
>>  * unconditionally call the cleanup code.  To avoid deadlock, only
>>  * call this after closing the fd resulting from virFileWrapperFdNew().
>>  *
>>  * This function can be safely called multiple times on the same @wfd.
>>  */
>>
>> Seems it has been specifically designed to simplify situations like this.
>>

Hi Ani, did you read the comments snippet above?

To me it looks like there is nothing else to do here, because someone thought about this,
and made the wrapper API easier to use.

Thanks,

Claudio

On Wed, Apr 13, 2022 at 11:57 AM Claudio Fontana <cfontana@suse.de> wrote:
>
> On 4/13/22 6:13 AM, Ani Sinha wrote:
> > On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana <cfontana@suse.de> wrote:
> >>
> >> On 4/12/22 7:23 PM, Ani Sinha wrote:
> >>> On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani@anisinha.ca> wrote:
> >>>
> >>>> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@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;
> >>>>> +    }
> >>>>
> >>>> There is an obvious issue with this code. We are trying to close and
> >>>> free a file descriptor that we have not opened when
> >>>> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
> >>>
> >>>
> >>> I meant *not* set in flags.
> >>
> >> I don't think so. I don't think it is obvious, so can you be more specific?
> >
> > See
> >    if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
> >         goto cleanup;
> >
> > and under cleanup:
> >    if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
> >         ret = -1;
> >
> > if VIR_DOMAIN_SAVE_BYPASS_CACHE not set in flags, both the above
> > function calls use wrapperFd initialized to NULL.
> > I think this patch would be incomplete if you do not handle all these scenarios.
> >
> >>
> >> From the function header comments it seems lecit and defined to call the function with NULL:
> >>
> >> /**
> >>  * virFileWrapperFdClose:
> >>  * @wfd: fd wrapper, or NULL
> >>  *
> >>  * If @wfd is valid, then ensure that I/O has completed, which may
> >>  * include reaping a child process.  Return 0 if all data for the
> >>  * wrapped fd is complete, or -1 on failure with an error emitted.
> >>  * This function intentionally returns 0 when @wfd is NULL, so that
> >>  * callers can conditionally create a virFileWrapperFd wrapper but
> >>  * unconditionally call the cleanup code.  To avoid deadlock, only
> >>  * call this after closing the fd resulting from virFileWrapperFdNew().
> >>  *
> >>  * This function can be safely called multiple times on the same @wfd.
> >>  */
> >>
> >> Seems it has been specifically designed to simplify situations like this.
> >>
>
> Hi Ani, did you read the comments snippet above?

yes and I get that the called functions today are written with safety
for such cases in mind (check for null file descriptors). However, I
still think it is quite unclean (and looks buggy) to do stuff that
does not apply. With this change, wrapperFd only used when
VIR_DOMAIN_SAVE_BYPASS_CACHE is set and we should avoid any operations
on the fd when VIR_DOMAIN_SAVE_BYPASS_CACHE is not set.
That being said, previous to this patch, under error conditions from
call to virFileWrapperFdNew() also would call close() and free()
without checks for null fd from cleanup. So I guess we are not at
least making things any worse by not checking for null fd from the
caller of those two functions.

On 4/13/22 8:57 AM, Ani Sinha wrote:
> On Wed, Apr 13, 2022 at 11:57 AM Claudio Fontana <cfontana@suse.de> wrote:
>>
>> On 4/13/22 6:13 AM, Ani Sinha wrote:
>>> On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana <cfontana@suse.de> wrote:
>>>>
>>>> On 4/12/22 7:23 PM, Ani Sinha wrote:
>>>>> On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani@anisinha.ca> wrote:
>>>>>
>>>>>> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@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;
>>>>>>> +    }
>>>>>>
>>>>>> There is an obvious issue with this code. We are trying to close and
>>>>>> free a file descriptor that we have not opened when
>>>>>> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
>>>>>
>>>>>
>>>>> I meant *not* set in flags.
>>>>
>>>> I don't think so. I don't think it is obvious, so can you be more specific?
>>>
>>> See
>>>    if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
>>>         goto cleanup;
>>>
>>> and under cleanup:
>>>    if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
>>>         ret = -1;
>>>
>>> if VIR_DOMAIN_SAVE_BYPASS_CACHE not set in flags, both the above
>>> function calls use wrapperFd initialized to NULL.
>>> I think this patch would be incomplete if you do not handle all these scenarios.
>>>
>>>>
>>>> From the function header comments it seems lecit and defined to call the function with NULL:
>>>>
>>>> /**
>>>>  * virFileWrapperFdClose:
>>>>  * @wfd: fd wrapper, or NULL
>>>>  *
>>>>  * If @wfd is valid, then ensure that I/O has completed, which may
>>>>  * include reaping a child process.  Return 0 if all data for the
>>>>  * wrapped fd is complete, or -1 on failure with an error emitted.
>>>>  * This function intentionally returns 0 when @wfd is NULL, so that
>>>>  * callers can conditionally create a virFileWrapperFd wrapper but
>>>>  * unconditionally call the cleanup code.  To avoid deadlock, only
>>>>  * call this after closing the fd resulting from virFileWrapperFdNew().
>>>>  *
>>>>  * This function can be safely called multiple times on the same @wfd.
>>>>  */
>>>>
>>>> Seems it has been specifically designed to simplify situations like this.
>>>>
>>
>> Hi Ani, did you read the comments snippet above?
> 
> yes and I get that the called functions today are written with safety
> for such cases in mind (check for null file descriptors). However, I
> still think it is quite unclean (and looks buggy) to do stuff that
> does not apply. With this change, wrapperFd only used when
> VIR_DOMAIN_SAVE_BYPASS_CACHE is set and we should avoid any operations
> on the fd when VIR_DOMAIN_SAVE_BYPASS_CACHE is not set.
> That being said, previous to this patch, under error conditions from
> call to virFileWrapperFdNew() also would call close() and free()
> without checks for null fd from cleanup. So I guess we are not at
> least making things any worse by not checking for null fd from the
> caller of those two functions.
> 

It is a curious approach I agree, but I think this is how libvirt wants it, based on specifically this part of the comment in the code:

"
 This function intentionally returns 0 when @wfd is NULL, so that
 * callers can conditionally create a virFileWrapperFd wrapper but
 * unconditionally call the cleanup code. 
"

Thanks,

Claudio

On Wed, Apr 13, 2022 at 9:43 AM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana <cfontana@suse.de> wrote:
> >
> > On 4/12/22 7:23 PM, Ani Sinha wrote:
> > > On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > >> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@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;
> > >>> +    }
> > >>
> > >> There is an obvious issue with this code. We are trying to close and
> > >> free a file descriptor that we have not opened when
> > >> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
> > >
> > >
> > > I meant *not* set in flags.
> >
> > I don't think so. I don't think it is obvious, so can you be more specific?
>
> See
>    if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
>         goto cleanup;
>
> and under cleanup:
>    if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
>         ret = -1;

Also see virFileWrapperFdFree() in cleanup.

>
> if VIR_DOMAIN_SAVE_BYPASS_CACHE not set in flags, both the above
> function calls use wrapperFd initialized to NULL.
> I think this patch would be incomplete if you do not handle all these scenarios.
>
> >
> > From the function header comments it seems lecit and defined to call the function with NULL:
> >
> > /**
> >  * virFileWrapperFdClose:
> >  * @wfd: fd wrapper, or NULL
> >  *
> >  * If @wfd is valid, then ensure that I/O has completed, which may
> >  * include reaping a child process.  Return 0 if all data for the
> >  * wrapped fd is complete, or -1 on failure with an error emitted.
> >  * This function intentionally returns 0 when @wfd is NULL, so that
> >  * callers can conditionally create a virFileWrapperFd wrapper but
> >  * unconditionally call the cleanup code.  To avoid deadlock, only
> >  * call this after closing the fd resulting from virFileWrapperFdNew().
> >  *
> >  * This function can be safely called multiple times on the same @wfd.
> >  */
> >
> > Seems it has been specifically designed to simplify situations like this.
> >
> > Thanks,
> >
> > Claudio
> >
> >
> > >
> > >
> > >>
> > >>>
> > >>>      if (virQEMUSaveDataWrite(data, fd, path) < 0)
> > >>>          goto cleanup;
> > >>> --
> > >>> 2.34.1
> > >>>
> > >>
> > >
> >

On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@suse.de>

Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  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;
> +    }
>
>      if (virQEMUSaveDataWrite(data, fd, path) < 0)
>          goto cleanup;
> --
> 2.34.1
>

a ping on this one;

this change makes sense to me, and makes performance better in specific cases, by avoiding --bypass-cache before reaching the "cache trashing" state.

However, before virsh save was guarenteed to go through iohelper, now it is not.

The iohelper was guaranteeing a virFileDataSync before exiting,
while after the change the data will still be in-flight between the kernel and the device as virsh save returns to the prompt.

Seems more efficient, but wanted to mention this as a user-visible change.

Thoughts?

Claudio


On 4/13/22 9:18 AM, Ani Sinha wrote:
> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@suse.de> 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@suse.de>
> 
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> 
>> ---
>>  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;
>> +    }
>>
>>      if (virQEMUSaveDataWrite(data, fd, path) < 0)
>>          goto cleanup;
>> --
>> 2.34.1
>>
>

On Thu, Apr 21, 2022 at 04:55:53PM +0200, Claudio Fontana wrote:
> a ping on this one;
> 
> this change makes sense to me, and makes performance better in specific cases, by avoiding --bypass-cache before reaching the "cache trashing" state.
> 
> However, before virsh save was guarenteed to go through iohelper, now it is not.
> 
> The iohelper was guaranteeing a virFileDataSync before exiting,
> while after the change the data will still be in-flight between
> the kernel and the device as virsh save returns to the prompt.

This is a correctness problem I'm afraid, notably with network
filesystems

commit f32e3a2dd686f3692cd2bd3147c03e90f82df987
Author: Michal Prívozník <mprivozn@redhat.com>
Date:   Tue Oct 30 19:15:48 2012 +0100

    iohelper: fdatasync() at the end
    
    Currently, when we are doing (managed) save, we insert the
    iohelper between the qemu and OS. The pipe is created, the
    writing end is passed to qemu and the reading end to the
    iohelper. It reads data and write them into given file. However,
    with write() being asynchronous data may still be in OS
    caches and hence in some (corner) cases, all migration data
    may have been read and written (not physically though). So
    qemu will report success, as well as iohelper. However, with
    some non local filesystems, where ENOSPACE is polled every X
    time units, we may get into situation where all operations
    succeeded but data hasn't reached the disk. And in fact will
    never do. Therefore we ought sync caches to make sure data
    has reached the block device on remote host.


It is also a crash-safety issue. ie if the host OS crashes
after we've created the save image, the image will still
exist on disk but can have lost arbitrary amounts of data,
in a way they might not be detectable until the restored
VM randomly crashes hours/days later.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|