[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@xxxxxxxxx> > > Base upon patches from Roy Keene <rkeene@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx> > Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxx> > --- > 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); > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
