* Peter Xu (pet...@redhat.com) wrote: > On Tue, Mar 23, 2021 at 08:21:43PM +0300, Andrey Gruzdev wrote: > > > For the long term I think we'd better have a helper: > > > > > > qemu_put_qio_channel_buffer(QEMUFile *file, QIOChannelBuffer > > > *bioc) > > > > > > So as to hide this flush operation, which is tricky. We'll have two users > > > so > > > far: > > > > > > bg_migration_completion > > > colo_do_checkpoint_transaction > > > > > > IMHO it'll be nicer if you'd do it in this patch altogether! > > > > > > Thanks, > > > > > Sorry, can't get the idea, what's wrong with the fix. > > I'm fine with the fix, but I've got one patch attached just to show what I > meant, so without any testing for sure.. > > Looks more complicated than I thought, but again I think we should hide that > buffer flush into another helper to avoid overlooking it.
I was wondering if I was missing the same fflush in postcopy, but I don't *think* so, although it's a bit round about; before sending the data I call: qemu_savevm_send_postcopy_run(fb) and that calls qemu_savevm_command_send that ends in a fflish; which is non-obvious. While I'd leave that in there, it might be good to use that same thing. Dave > Thanks, > > -- > Peter Xu > From f3004948e2498fb9ac4646a6a5225c58ea3f1623 Mon Sep 17 00:00:00 2001 > From: Peter Xu <pet...@redhat.com> > Date: Tue, 23 Mar 2021 14:30:24 -0400 > Subject: [PATCH] migration: Introduce qemu_put_qio_channel_buffer() > > Meanwhile use it in background snapshot code, so as to dump one QEMUFile > buffer > (which is created by QIOChannelBuffer) into another QEMUFile. > > Similar thing should be able to be applied to colo_do_checkpoint_transaction() > too. > > Signed-off-by: Peter Xu <pet...@redhat.com> > --- > migration/migration.c | 16 +++++++++------- > migration/migration.h | 1 - > migration/qemu-file-channel.c | 14 ++++++++++++++ > migration/qemu-file-channel.h | 2 ++ > migration/qemu-file.c | 4 ++++ > migration/qemu-file.h | 1 + > 6 files changed, 30 insertions(+), 8 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index a5ddf43559e..9d73c236b16 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -3243,8 +3243,9 @@ fail: > * RAM has been saved. The caller 'breaks' the loop when this returns. > * > * @s: Current migration state > + * @vmstates: The QEMUFile* buffer that contains all the device vmstates > */ > -static void bg_migration_completion(MigrationState *s) > +static void bg_migration_completion(MigrationState *s, QEMUFile *vmstates) > { > int current_active_state = s->state; > > @@ -3262,7 +3263,7 @@ static void bg_migration_completion(MigrationState *s) > * right after the ram content. The device state has been stored into > * the temporary buffer before RAM saving started. > */ > - qemu_put_buffer(s->to_dst_file, s->bioc->data, s->bioc->usage); > + qemu_put_qio_channel_buffer(s->to_dst_file, vmstates); > qemu_fflush(s->to_dst_file); > } else if (s->state == MIGRATION_STATUS_CANCELLING) { > goto fail; > @@ -3656,7 +3657,6 @@ static MigIterateState > bg_migration_iteration_run(MigrationState *s) > > res = qemu_savevm_state_iterate(s->to_dst_file, false); > if (res > 0) { > - bg_migration_completion(s); > return MIG_ITERATE_BREAK; > } > > @@ -3843,6 +3843,7 @@ static void *bg_migration_thread(void *opaque) > MigThrError thr_error; > QEMUFile *fb; > bool early_fail = true; > + QIOChannelBuffer *bioc; > > rcu_register_thread(); > object_ref(OBJECT(s)); > @@ -3859,10 +3860,10 @@ static void *bg_migration_thread(void *opaque) > * with vCPUs running and, finally, write stashed non-RAM part of > * the vmstate from the buffer to the migration stream. > */ > - s->bioc = qio_channel_buffer_new(128 * 1024); > - qio_channel_set_name(QIO_CHANNEL(s->bioc), "vmstate-buffer"); > - fb = qemu_fopen_channel_output(QIO_CHANNEL(s->bioc)); > - object_unref(OBJECT(s->bioc)); > + bioc = qio_channel_buffer_new(128 * 1024); > + qio_channel_set_name(QIO_CHANNEL(bioc), "vmstate-buffer"); > + fb = qemu_fopen_channel_output(QIO_CHANNEL(bioc)); > + object_unref(OBJECT(bioc)); > > update_iteration_initial_status(s); > > @@ -3935,6 +3936,7 @@ static void *bg_migration_thread(void *opaque) > if (iter_state == MIG_ITERATE_SKIP) { > continue; > } else if (iter_state == MIG_ITERATE_BREAK) { > + bg_migration_completion(s, fb); > break; > } > > diff --git a/migration/migration.h b/migration/migration.h > index db6708326b1..bdcd74ce084 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -151,7 +151,6 @@ struct MigrationState { > QEMUBH *vm_start_bh; > QEMUBH *cleanup_bh; > QEMUFile *to_dst_file; > - QIOChannelBuffer *bioc; > /* > * Protects to_dst_file pointer. We need to make sure we won't > * yield or hang during the critical section, since this lock will > diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c > index afc3a7f642a..c1bf71510f8 100644 > --- a/migration/qemu-file-channel.c > +++ b/migration/qemu-file-channel.c > @@ -26,6 +26,7 @@ > #include "qemu-file-channel.h" > #include "qemu-file.h" > #include "io/channel-socket.h" > +#include "io/channel-buffer.h" > #include "qemu/iov.h" > #include "qemu/yank.h" > > @@ -196,3 +197,16 @@ QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc) > object_ref(OBJECT(ioc)); > return qemu_fopen_ops(ioc, &channel_output_ops); > } > + > +/* > + * Splice all the data in `buffer' into `dst'. Note that `buffer' must > points > + * to a QEMUFile that was created with qemu_fopen_channel_output(). > + */ > +void qemu_put_qio_channel_buffer(QEMUFile *dst, QEMUFile *buffer) > +{ > + QIOChannelBuffer *bioc = > QIO_CHANNEL_BUFFER(qemu_file_get_opaque(buffer)); > + > + /* Make sure data cached in QEMUFile flushed to QIOChannel buffers */ > + qemu_fflush(buffer); > + qemu_put_buffer(dst, bioc->data, bioc->usage); > +} > diff --git a/migration/qemu-file-channel.h b/migration/qemu-file-channel.h > index 0028a09eb61..5f165527616 100644 > --- a/migration/qemu-file-channel.h > +++ b/migration/qemu-file-channel.h > @@ -29,4 +29,6 @@ > > QEMUFile *qemu_fopen_channel_input(QIOChannel *ioc); > QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc); > +void qemu_put_qio_channel_buffer(QEMUFile *dst, QEMUFile *buffer); > + > #endif > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index d6e03dbc0e0..317f98fc8f5 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -112,6 +112,10 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps > *ops) > return f; > } > > +void *qemu_file_get_opaque(QEMUFile *f) > +{ > + return f->opaque; > +} > > void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks) > { > diff --git a/migration/qemu-file.h b/migration/qemu-file.h > index a9b6d6ccb7d..30c8ec855ac 100644 > --- a/migration/qemu-file.h > +++ b/migration/qemu-file.h > @@ -161,6 +161,7 @@ int qemu_file_shutdown(QEMUFile *f); > QEMUFile *qemu_file_get_return_path(QEMUFile *f); > void qemu_fflush(QEMUFile *f); > void qemu_file_set_blocking(QEMUFile *f, bool block); > +void *qemu_file_get_opaque(QEMUFile *f); > > void ram_control_before_iterate(QEMUFile *f, uint64_t flags); > void ram_control_after_iterate(QEMUFile *f, uint64_t flags); > -- > 2.26.2 > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK