Re: [libvirt RFC] qemu_saveimage: only use wrapper when using --bypass-cache
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>
wrote:
> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana(a)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(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;
>> + }
>
> 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
>>
>