On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana(a)suse.de&gt; 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&gt; > --- > 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 4/12/22 7:23 PM, Ani Sinha wrote: > On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani(a)anisinha.ca&gt; wrote: > >> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana(a)suse.de&gt; 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&gt; >>> --- >>> 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(a)suse.de&gt; wrote: > > On 4/12/22 7:23 PM, Ani Sinha wrote: >> On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani(a)anisinha.ca&gt; wrote: >> >>> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana(a)suse.de&gt; 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&gt; >>>> --- >>>> 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. >

On Wed, Apr 13, 2022 at 11:57 AM Claudio Fontana <cfontana(a)suse.de&gt; wrote: > > On 4/13/22 6:13 AM, Ani Sinha wrote: >> On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana <cfontana(a)suse.de&gt; wrote: >>> >>> On 4/12/22 7:23 PM, Ani Sinha wrote: >>>> On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani(a)anisinha.ca&gt; wrote: >>>> >>>>> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana(a)suse.de&gt; 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&gt; >>>>>> --- >>>>>> 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 Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana(a)suse.de&gt; 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&gt; Reviewed-by: Ani Sinha <ani(a)anisinha.ca&gt; > --- > 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 >

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&gt; --- 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; + }

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(a)suse.de&gt; >> --- >> 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&gt; > 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