[libvirt] [PATCH v3 1/2] qemu: Allow qemuDomainSaveMemory saving VM state to a pipe
John Ferlan
jferlan at redhat.com
Thu Aug 3 00:24:42 UTC 2017
[pardon the top post]
Doing some (personal) mail box cleaning and found this "older" patch...
Sorry that this has sat unattended (and probably now forgotten or given
up on) for so long. I don't even recall v1 or v2 reviews - been too
long. In any case, if you still would like to see this addressed, could
you rebase on top of tree and send again along with a few points below...
On 03/08/2017 11:22 PM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao at gmail.com>
>
> Base upon patches from Roy Keene <rkeene at knightpoint.com>
>
> Currently qemuDomainSaveMemory can save vm's config
> and memory to fd.
> It write a magic QEMU_SAVE_PARTIAL firstly,
> then re-open it to change QEMU_SAVE_PARTIAL as QEMU_SAVE_MAGIC.
>
> For pipes this is not possible, attempting to re-open the pipe
> will not connect you to the same consumer.
> Seeking is also not possible on a pipe.
>
> This patch introduce VIR_DOMAIN_SAVE_DIRECT
> If set, write QEMU_SAVE_MAGIC directly.
>
> This is useful to me for saving a VM state directly to
> Ceph RBD images without having an intermediate file.
The above really needs to be cleaned up to be less choppy. I see some of
this is copied later too
>
> Signed-off-by: Roy Keene <rkeene at knightpoint.com>
> Signed-off-by: Chen Hanxiao <chenhanxiao at gmail.com>
> ---
> v3:
> add news.xml
>
> v2-resend:
> rebase on upstream
>
> v2:
> rename VIR_DOMAIN_SAVE_PIPE to VIR_DOMAIN_SAVE_DIRECT
> remove S_ISFIFO check for dst path
>
> docs/news.xml | 9 +++++++
> include/libvirt/libvirt-domain.h | 1 +
> src/qemu/qemu_driver.c | 54 ++++++++++++++++++++++++++--------------
> 3 files changed, 46 insertions(+), 18 deletions(-)
>
You'll need to understand the changes in the code made more recently for
commit ids 'a2d2aae14' and 'ec986bc5' which altered the flow of the code
to make things more common...
I had to go all the way back to Dec archives to find any comment
regarding this series:
https://www.redhat.com/archives/libvir-list/2016-December/msg00318.html
and before that Roy's series:
https://www.redhat.com/archives/libvir-list/2016-November/msg00269.html
So while I know it's important to have some continuity for some reasons
vis-a-vis the subject line; however, at this point I think you just need
to make a fresh subject indicating the ability to save 'directly'. Using
a pipe just happens to be an implementation of that concept. You can
and should include pointers to the archives of the previous series. It
helps set context for the reviewer when a v4 arrives in which they don't
recognize the subject.
> diff --git a/docs/news.xml b/docs/news.xml
> index 9515395..57088db 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -37,6 +37,15 @@
> applications running on the platform.
> </description>
> </change>
> + <change>
> + <summary>
> + qemu: Allow qemuDomainSaveMemory saving VM state to a pipe> + </summary>
> + <description>
> + Introduce flag VIR_DOMAIN_SAVE_DIRECT to enable command 'save'
> + to write to PIPE, for PIPE can't be reopened.
> + </description>
> + </change>
> </section>
> <section title="Bug fixes">
> <change/>
This should be its own separate third patch.
Still you should reword to avoid directly mentioning pipe or fifo, but
using directly to some already opened target. Perhaps in the description
you can mention that by directly, this means you could use a pipe and
provide that example (but try to do so without ceph). IOW: How would
someone use this in a general sense. Would some sort of socket be
another way to use this?
The difference w/ direct is that the to be saved "save-image"
destination doesn't necessarily need to be to a file, it can be to
something else via a pipe or some other indirection, right? (IIUC at least)
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index c490d71..f58fe2c 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1169,6 +1169,7 @@ typedef enum {
> VIR_DOMAIN_SAVE_BYPASS_CACHE = 1 << 0, /* Avoid file system cache pollution */
> VIR_DOMAIN_SAVE_RUNNING = 1 << 1, /* Favor running over paused */
> VIR_DOMAIN_SAVE_PAUSED = 1 << 2, /* Favor paused over running */
> + VIR_DOMAIN_SAVE_DIRECT = 1 << 3, /* Write QEMU_SAVE_MAGIC directly */
I'd probably avoid mentioning QEMU_SAVE_MAGIC...
> } virDomainSaveRestoreFlags;
>
> int virDomainSave (virDomainPtr domain,
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2032fac..29b7677 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3059,6 +3059,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
> virQEMUSaveHeader header;
> bool bypassSecurityDriver = false;
> bool needUnlink = false;
> + bool canReopen = true;
I think I would have gone with something like:
bool isDirect = (flags & VIR_DOMAIN_SAVE_DIRECT);
> int ret = -1;
> int fd = -1;
> int directFlag = 0;
> @@ -3066,7 +3067,6 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
> unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
hmmm... NON_BLOCKING - that's triggering something I looked at
recently... But how would --bypass-cache affect things for --direct?
>
> memset(&header, 0, sizeof(header));
> - memcpy(header.magic, QEMU_SAVE_PARTIAL, sizeof(header.magic));
> header.version = QEMU_SAVE_VERSION;
> header.was_running = was_running ? 1 : 0;
> header.compressed = compressed;
> @@ -3082,6 +3082,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
> goto cleanup;
> }
> }
> +
Unrelated.
> fd = qemuOpenFile(driver, vm, path,
> O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> &needUnlink, &bypassSecurityDriver);
> @@ -3094,6 +3095,20 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
> if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
> goto cleanup;
>
> + /* Set the header magic.
> + * Setting flags VIR_DOMAIN_SAVE_DIRECT will write
> + * magic QEMU_SAVE_MAGIC directly.
> + * For PIPE, we should do this because it can't be reopen.
> + * Otherwise we'll update the magic after
> + * the saving completes successfully.
> + */
The comment is "choppy", but I pointed that out above.
> + if (flags & VIR_DOMAIN_SAVE_DIRECT) {
if (isDirect)
memcpy MAGIC
else
mempcy PARTIAL
of course this code changed, so you'll need to see how you're affected.
> + canReopen = false;
> + memcpy(header.magic, QEMU_SAVE_MAGIC, sizeof(header.magic));
> + } else {
> + memcpy(header.magic, QEMU_SAVE_PARTIAL, sizeof(header.magic));
> + }
> +
> /* Write header to file, followed by XML */
> if (qemuDomainSaveHeader(fd, path, domXML, &header) < 0)
> goto cleanup;
> @@ -3102,28 +3117,30 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
> if (qemuMigrationToFile(driver, vm, fd, compressedpath, asyncJob) < 0)
> goto cleanup;
>
Then add a check here:
if (isDirect) {
ret = 0;
goto cleanup;
}
Leaving the next pile untouched
> - /* Touch up file header to mark image complete. */
> + if (canReopen) {
> + /* Touch up file header to mark image complete. */
>
> - /* Reopen the file to touch up the header, since we aren't set
> - * up to seek backwards on wrapperFd. The reopened fd will
> - * trigger a single page of file system cache pollution, but
> - * that's acceptable. */
> - if (VIR_CLOSE(fd) < 0) {
> - virReportSystemError(errno, _("unable to close %s"), path);
> - goto cleanup;
> - }
> + /* Reopen the file to touch up the header, since we aren't set
> + * up to seek backwards on wrapperFd. The reopened fd will
> + * trigger a single page of file system cache pollution, but
> + * that's acceptable. */
Notice how your comments didn't line up here. Won't matter if you use
the goto I suggest above.
Still I wonder about the viability of using a pipe for something
sufficiently large or something that gets interrupted. Could having the
QEMU_SAVE_MAGIC in the header cause "some other" mechanism to believe
it has a completely written file? IOW: How does something else know
it's done? Does that matter? Perhaps not for the case you're using, but
in a more general sense.
I don't know the save/restore code all that well, but I know save does
use iohelper and IIUC the whole purpose of PARTIAL and MAGIC is to
somehow indicate to a different operation the state of the 'file' being
saved.
I responded recently to someone having a patch to use iohelper on
restore, not sure if what I wrote there will help or not, but it does
have some pointers to patches for iohelper/cache processing, see:
https://www.redhat.com/archives/libvir-list/2017-July/msg00176.html
Again save/restore processing is not something I've studied that
closely, but my main concern is there are two known states - MAGIC
(complete) and PARTIAL (incomplete) that other code may make some
assumptions about that you'd be circumventing by immediately setting
MAGIC (done). I may be off base, but I figured I'd at least mention it
to make sure it's felt it's not a problem in this instance.
John
> + if (VIR_CLOSE(fd) < 0) {
> + virReportSystemError(errno, _("unable to close %s"), path);
> + goto cleanup;
> + }
>
> - if (virFileWrapperFdClose(wrapperFd) < 0)
> - goto cleanup;
> + if (virFileWrapperFdClose(wrapperFd) < 0)
> + goto cleanup;
>
> - if ((fd = qemuOpenFile(driver, vm, path, O_WRONLY, NULL, NULL)) < 0)
> - goto cleanup;
> + if ((fd = qemuOpenFile(driver, vm, path, O_WRONLY, NULL, NULL)) < 0)
> + goto cleanup;
>
> - memcpy(header.magic, QEMU_SAVE_MAGIC, sizeof(header.magic));
> + memcpy(header.magic, QEMU_SAVE_MAGIC, sizeof(header.magic));
>
> - if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) {
> - virReportSystemError(errno, _("unable to write %s"), path);
> - goto cleanup;
> + if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) {
> + virReportSystemError(errno, _("unable to write %s"), path);
> + goto cleanup;
> + }
> }
>
> if (VIR_CLOSE(fd) < 0) {
> @@ -3353,6 +3370,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml,
>
> virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE |
> VIR_DOMAIN_SAVE_RUNNING |
> + VIR_DOMAIN_SAVE_DIRECT |
> VIR_DOMAIN_SAVE_PAUSED, -1);
>
> cfg = virQEMUDriverGetConfig(driver);
>
More information about the libvir-list
mailing list