On 01/27/2017 05:59 AM, Shivaprasad G Bhat wrote:
> Commit afe6e58 & c4caab53 made necessary changes to use io-helpers
> during save and restore. The commit c4caab53 missed to remove the
> redundant check in qemuDomainSaveImageOpen() because of which
> virFileWrapperFdNew() is not called if bypass_cache is false.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@xxxxxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
Since it's been sitting for a while with a couple of pings...
First off - it doesn't apply on top of the current git tree. Secondly,
not an area of the code I've worked in, but perhaps a response will at
least cause others to chime in...
Beyond that, the commit messages noted above are really, really old.
Still in looking at them "quickly" they are handling the Save operation
and not the Restore operation AFAICT, so I'm not sure your commit
message is precise.
What this patch is trying to do is apply the same/similar logic for the
Restore operation that the Save operation via qemuDomainSaveMemory is
doing - is that a fair assessment? But what I'm not clear of is what
problem is trying to be solved. What follows is my interpretation from
looking at the code...
There are 4 callers to SaveImageOpen, two (qemuDomainSaveImageGetXMLDesc
and qemuDomainSaveImageDefineXML) can be ruled out quickly because they
pass bypass_cache = false and wrapperFd = NULL.
That leaves qemuDomainRestoreFlags which will set bypass_cache depending
on whether or not the VIR_DOMAIN_SAVE_BYPASS_CACHE flag was set and
qemuDomainObjRestore which has a similar setting based on whether
qemuDomainObjStart had VIR_DOMAIN_START_BYPASS_CACHE set.
All of that seems perfectly reasonable. So, what then does this patch
do. Well as long as a valid wrapperFd was passed, it would then always
call virFileWrapperFdNew with at least the VIR_FILE_WRAPPER_NON_BLOCKING
flag set; however, if one checks the virFileWrapperFdNew comments that
should only be set if non-blocking I/O is properly supported. Still
nothing is yet done with that flag, so again no big deal (there's a
comment about posix_fadvise). At least we know for now that open_write
will be false from the two paths in question; otherwise, the other
requirement in virFileWrapperFdNew that the fd be O_RDONLY or O_WRONLY
wouldn't be met.
So the question is does the iohelper really need to always be used on
Restore when bypass_cache isn't set? And of course why? IIRC, using
iohelper was to avoid blocking other qemu interactions. But Restore is
pulling from a file and doesn't have those same interactions - so what
advantage is using iohelper for the Restore function that doesn't have
bypass-cache set?
John
BTW: There's another side effect... On a platform where
virFileWrapperFdNew isn't supported, Restore wuld fail always. Not that
Save would work for the same reason, but one wouldn't be able to Restore.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 516a851..ac89372 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6150,9 +6150,11 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
> virDomainDefPtr def = NULL;
> int oflags = open_write ? O_RDWR : O_RDONLY;
> virCapsPtr caps = NULL;
> + unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
>
> if (bypass_cache) {
> int directFlag = virFileDirectFdFlag();
> + wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
> if (directFlag < 0) {
> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> _("bypass cache unsupported by this system"));
> @@ -6166,9 +6168,8 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>
> if ((fd = qemuOpenFile(driver, NULL, path, oflags, NULL, NULL)) < 0)
> goto error;
> - if (bypass_cache &&
> - !(*wrapperFd = virFileWrapperFdNew(&fd, path,
> - VIR_FILE_WRAPPER_BYPASS_CACHE)))
> + if (wrapperFd &&
> + !(*wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
> goto error;
>
> if (saferead(fd, &header, sizeof(header)) != sizeof(header)) {
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
