| Message ID | 20240301091524.39900-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/multifd: Document two places for mapped-ram | expand |
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>
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, ...) 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
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,
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
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(); }