migration/multifd.c | 12 ++++++++++++ migration/ram.c | 8 +++++++- 2 files changed, 19 insertions(+), 1 deletion(-)
From: Peter Xu <peterx@redhat.com>
Add two documentations for mapped-ram migration on two spots that may not
be extremely clear.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
Based-on: <20240229153017.2221-1-farosas@suse.de>
---
migration/multifd.c | 12 ++++++++++++
migration/ram.c | 8 +++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index b4e5a9dfcc..2942395ce2 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -709,6 +709,18 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
{
if (p->c) {
migration_ioc_unregister_yank(p->c);
+ /*
+ * An explicitly close() on the channel here is normally not
+ * required, but can be helpful for "file:" iochannels, where it
+ * will include an fdatasync() to make sure the data is flushed to
+ * the disk backend.
+ *
+ * The object_unref() cannot guarantee that because: (1) finalize()
+ * of the iochannel is only triggered on the last reference, and
+ * it's not guaranteed that we always hold the last refcount when
+ * reaching here, and, (2) even if finalize() is invoked, it only
+ * does a close(fd) without data flush.
+ */
qio_channel_close(p->c, &error_abort);
object_unref(OBJECT(p->c));
p->c = NULL;
diff --git a/migration/ram.c b/migration/ram.c
index 1f1b5297cf..c79e3de521 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4258,7 +4258,13 @@ static int ram_load_precopy(QEMUFile *f)
switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
case RAM_SAVE_FLAG_MEM_SIZE:
ret = parse_ramblocks(f, addr);
-
+ /*
+ * For mapped-ram migration (to a file) using multifd, we sync
+ * once and for all here to make sure all tasks we queued to
+ * multifd threads are completed, so that all the ramblocks
+ * (including all the guest memory pages within) are fully
+ * loaded after this sync returns.
+ */
if (migrate_mapped_ram()) {
multifd_recv_sync_main();
}
--
2.44.0Hello Petr,
On Fri, 1 Mar 2024 at 14:46, <peterx@redhat.com> wrote:
> + * An explicitly close() on the channel here is normally not
explicitly -> explicit
> + * required, but can be helpful for "file:" iochannels, where it
> + * will include an fdatasync() to make sure the data is flushed to
> + * the disk backend.
* an fdatasync() -> fdatasync()
* qio_channel_close
-> ioc_klass->io_close = qio_channel_file_close;
-> qemu_close(fioc->fd)
-> close(fd);
It does not seem to call fdatasync() before close(fd);
- qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, ...)
Maybe the qio_channel..() calls above should include the 'O_DSYNC'
flag as well? But that will do fdatasync() work at each write(2) call
I think, not sure if that is okay.
> + *
> + * The object_unref() cannot guarantee that because: (1) finalize()
> + * of the iochannel is only triggered on the last reference, and
> + * it's not guaranteed that we always hold the last refcount when
> + * reaching here, and, (2) even if finalize() is invoked, it only
> + * does a close(fd) without data flush.
> + */
* object_unref
-> object_finalize
-> object_deinit
-> type->instance_finalize
-> qio_channel_file_finalize
-> qemu_close(ioc->fd);
* I hope I'm looking at the right code here. (Sorry if I'm not)
Thank you.
---
- Prasad
On Fri, Mar 01, 2024 at 11:17:10PM +0530, Prasad Pandit wrote: > Hello Petr, > > On Fri, 1 Mar 2024 at 14:46, <peterx@redhat.com> wrote: > > + * An explicitly close() on the channel here is normally not > > explicitly -> explicit > > > + * required, but can be helpful for "file:" iochannels, where it > > + * will include an fdatasync() to make sure the data is flushed to > > + * the disk backend. > > * an fdatasync() -> fdatasync() > > * qio_channel_close > -> ioc_klass->io_close = qio_channel_file_close; > -> qemu_close(fioc->fd) > -> close(fd); > > It does not seem to call fdatasync() before close(fd); > > - qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, ...) The documented behaviour reliant on another pending patch: https://lists.nongnu.org/archive/html/qemu-devel/2024-02/msg07046.html > > Maybe the qio_channel..() calls above should include the 'O_DSYNC' > flag as well? But that will do fdatasync() work at each write(2) call > I think, not sure if that is okay. > > > + * > > + * The object_unref() cannot guarantee that because: (1) finalize() > > + * of the iochannel is only triggered on the last reference, and > > + * it's not guaranteed that we always hold the last refcount when > > + * reaching here, and, (2) even if finalize() is invoked, it only > > + * does a close(fd) without data flush. > > + */ > > * object_unref > -> object_finalize > -> object_deinit > -> type->instance_finalize > -> qio_channel_file_finalize > -> qemu_close(ioc->fd); > > * I hope I'm looking at the right code here. (Sorry if I'm not) With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Mar 01, 2024 at 05:50:24PM +0000, Daniel P. Berrangé wrote: > On Fri, Mar 01, 2024 at 11:17:10PM +0530, Prasad Pandit wrote: > > Hello Petr, Hey Prasad! Thanks for taking a look. > > > > On Fri, 1 Mar 2024 at 14:46, <peterx@redhat.com> wrote: > > > + * An explicitly close() on the channel here is normally not > > > > explicitly -> explicit > > > > > + * required, but can be helpful for "file:" iochannels, where it > > > + * will include an fdatasync() to make sure the data is flushed to > > > + * the disk backend. > > > > * an fdatasync() -> fdatasync() I'll fix both when apply. > > > > * qio_channel_close > > -> ioc_klass->io_close = qio_channel_file_close; > > -> qemu_close(fioc->fd) > > -> close(fd); > > > > It does not seem to call fdatasync() before close(fd); > > > > - qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, ...) > > The documented behaviour reliant on another pending patch: > > https://lists.nongnu.org/archive/html/qemu-devel/2024-02/msg07046.html Rightfully as Dan helped to answer. While for the other comment section in the same patch it relies on the other patch: https://lore.kernel.org/all/20240229153017.2221-20-farosas@suse.de/ > > > > > Maybe the qio_channel..() calls above should include the 'O_DSYNC' > > flag as well? But that will do fdatasync() work at each write(2) call > > I think, not sure if that is okay. > > > > > > > > + * > > > + * The object_unref() cannot guarantee that because: (1) finalize() > > > + * of the iochannel is only triggered on the last reference, and > > > + * it's not guaranteed that we always hold the last refcount when > > > + * reaching here, and, (2) even if finalize() is invoked, it only > > > + * does a close(fd) without data flush. > > > + */ > > > > * object_unref > > -> object_finalize > > -> object_deinit > > -> type->instance_finalize > > -> qio_channel_file_finalize > > -> qemu_close(ioc->fd); > > > > * I hope I'm looking at the right code here. (Sorry if I'm not) Yes I think you're looking at the right path, it's just that the relevant patches haven't yet landed upstream but is planned. I normally use "Based-on" tag for such patch that has a dependency outside master, like: Based-on: <20240229153017.2221-1-farosas@suse.de> I believe many others on the qemu list do the same. I think the rational is this will be both recognized by human beings and also by patchew, e.g. patchew will report a good apply status here: https://patchew.org/QEMU/20240301091524.39900-1-peterx@redhat.com/ And in the same patch if you fetch the tree patchew provided: git fetch https://github.com/patchew-project/qemu tags/patchew/20240301091524.39900-1-peterx@redhat.com You can also directly fetch the whole tree with this patch applied correctly on top of the dependency series. Personally I don't use patchew, but I kept that habit to declare patch dependencies just in case it'll help others who use it (e.g., I think patchew has other review tools like version comparisons, which I also don't use myself). Thanks, -- Peter Xu
On Mon, 4 Mar 2024 at 06:00, Peter Xu <peterx@redhat.com> wrote: > Yes I think you're looking at the right path, it's just that the relevant > patches haven't yet landed upstream but is planned. I normally use > "Based-on" tag for such patch that has a dependency outside master, like: > > Based-on: <20240229153017.2221-1-farosas@suse.de> > > I believe many others on the qemu list do the same. I think the rational > is this will be both recognized by human beings and also by patchew, > e.g. patchew will report a good apply status here: > > https://patchew.org/QEMU/20240301091524.39900-1-peterx@redhat.com/ > > And in the same patch if you fetch the tree patchew provided: > > git fetch https://github.com/patchew-project/qemu tags/patchew/20240301091524.39900-1-peterx@redhat.com > > You can also directly fetch the whole tree with this patch applied > correctly on top of the dependency series. > > Personally I don't use patchew, but I kept that habit to declare patch > dependencies just in case it'll help others who use it (e.g., I think > patchew has other review tools like version comparisons, which I also don't > use myself). * Interesting. Thank you. --- - Prasad
peterx@redhat.com writes: > From: Peter Xu <peterx@redhat.com> > > Add two documentations for mapped-ram migration on two spots that may not > be extremely clear. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
© 2016 - 2024 Red Hat, Inc.