Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread

[Dr. David Alan Gilbert]

* Peter Xu (peterx@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 <peterx@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 <peterx@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 / dgilbert@redhat.com / Manchester, UK