Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
On Wed, Mar 24, 2021 at 05:35:44PM +, Dr. David Alan Gilbert wrote: > * 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. Right, I was grepping qemu_put_buffer() previously, so as to easily got qemu_savevm_send_packaged() overlooked.. Maybe I can make it a small patch series after the snapshot fixes got in. Thanks, -- Peter Xu
Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
* 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
> 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
> ---
> 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;
>
Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
On 23.03.2021 21:35, Peter Xu 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. Thanks, Peter, now I've got what you meant - not to overlook flush on buffer channel. But it seems that adding a reasonable comment is enough here. Thanks,
Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
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.
Thanks,
--
Peter Xu
>From f3004948e2498fb9ac4646a6a5225c58ea3f1623 Mon Sep 17 00:00:00 2001
From: Peter Xu
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
---
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
Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
On 23.03.2021 17:54, Peter Xu wrote:
On Tue, Mar 23, 2021 at 10:51:57AM +0300, Andrey Gruzdev wrote:
On 22.03.2021 23:17, Peter Xu wrote:
On Fri, Mar 19, 2021 at 05:52:47PM +0300, Andrey Gruzdev wrote:
Added missing qemu_fflush() on buffer file holding precopy device state.
Increased initial QIOChannelBuffer allocation to 512KB to avoid reallocs.
Typical configurations often require >200KB for device state and VMDESC.
Signed-off-by: Andrey Gruzdev
---
migration/migration.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index ca8b97baa5..32b48fe9f5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3812,7 +3812,7 @@ 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);
+s->bioc = qio_channel_buffer_new(512 * 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));
@@ -3866,6 +3866,8 @@ static void *bg_migration_thread(void *opaque)
if (qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) {
goto fail;
}
+qemu_fflush(fb);
What will happen if the vmstates are bigger than 512KB? Would the extra data
be dropped?
No, the buffer shall be reallocated and the original content shall be kept.
Oh right..
Would you comment above qemu_fflush() about it (maybe also move it right before
the call to qemu_put_buffer)? Otherwise it indeed looks more like an
optimization rather than a bugfix.
Agree, better to have a comment here.
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.
Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
On Tue, Mar 23, 2021 at 10:51:57AM +0300, Andrey Gruzdev wrote:
> On 22.03.2021 23:17, Peter Xu wrote:
> > On Fri, Mar 19, 2021 at 05:52:47PM +0300, Andrey Gruzdev wrote:
> > > Added missing qemu_fflush() on buffer file holding precopy device state.
> > > Increased initial QIOChannelBuffer allocation to 512KB to avoid reallocs.
> > > Typical configurations often require >200KB for device state and VMDESC.
> > >
> > > Signed-off-by: Andrey Gruzdev
> > > ---
> > > migration/migration.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index ca8b97baa5..32b48fe9f5 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -3812,7 +3812,7 @@ 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);
> > > +s->bioc = qio_channel_buffer_new(512 * 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));
> > > @@ -3866,6 +3866,8 @@ static void *bg_migration_thread(void *opaque)
> > > if (qemu_savevm_state_complete_precopy_non_iterable(fb, false,
> > > false)) {
> > > goto fail;
> > > }
> > > +qemu_fflush(fb);
> > What will happen if the vmstates are bigger than 512KB? Would the extra
> > data
> > be dropped?
>
> No, the buffer shall be reallocated and the original content shall be kept.
Oh right..
Would you comment above qemu_fflush() about it (maybe also move it right before
the call to qemu_put_buffer)? Otherwise it indeed looks more like an
optimization rather than a bugfix.
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,
--
Peter Xu
Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
On 22.03.2021 23:17, Peter Xu wrote:
On Fri, Mar 19, 2021 at 05:52:47PM +0300, Andrey Gruzdev wrote:
Added missing qemu_fflush() on buffer file holding precopy device state.
Increased initial QIOChannelBuffer allocation to 512KB to avoid reallocs.
Typical configurations often require >200KB for device state and VMDESC.
Signed-off-by: Andrey Gruzdev
---
migration/migration.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index ca8b97baa5..32b48fe9f5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3812,7 +3812,7 @@ 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);
+s->bioc = qio_channel_buffer_new(512 * 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));
@@ -3866,6 +3866,8 @@ static void *bg_migration_thread(void *opaque)
if (qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) {
goto fail;
}
+qemu_fflush(fb);
What will happen if the vmstates are bigger than 512KB? Would the extra data
be dropped?
No, the buffer shall be reallocated and the original content shall be kept.
In that case, I'm wondering whether we'll need a qemu_file_get_error() after
the flush to detect it, and whether we need to retry with a bigger buffer size?
Thanks,
I think for QIOBufferChannel we don't need that, as I can see from the buffer
channel
implementation.
Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
On Fri, Mar 19, 2021 at 05:52:47PM +0300, Andrey Gruzdev wrote:
> Added missing qemu_fflush() on buffer file holding precopy device state.
> Increased initial QIOChannelBuffer allocation to 512KB to avoid reallocs.
> Typical configurations often require >200KB for device state and VMDESC.
>
> Signed-off-by: Andrey Gruzdev
> ---
> migration/migration.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index ca8b97baa5..32b48fe9f5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3812,7 +3812,7 @@ 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);
> +s->bioc = qio_channel_buffer_new(512 * 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));
> @@ -3866,6 +3866,8 @@ static void *bg_migration_thread(void *opaque)
> if (qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) {
> goto fail;
> }
> +qemu_fflush(fb);
What will happen if the vmstates are bigger than 512KB? Would the extra data
be dropped?
In that case, I'm wondering whether we'll need a qemu_file_get_error() after
the flush to detect it, and whether we need to retry with a bigger buffer size?
Thanks,
--
Peter Xu
[PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
Added missing qemu_fflush() on buffer file holding precopy device state.
Increased initial QIOChannelBuffer allocation to 512KB to avoid reallocs.
Typical configurations often require >200KB for device state and VMDESC.
Signed-off-by: Andrey Gruzdev
---
migration/migration.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index ca8b97baa5..32b48fe9f5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3812,7 +3812,7 @@ 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);
+s->bioc = qio_channel_buffer_new(512 * 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));
@@ -3866,6 +3866,8 @@ static void *bg_migration_thread(void *opaque)
if (qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) {
goto fail;
}
+qemu_fflush(fb);
+
/* Now initialize UFFD context and start tracking RAM writes */
if (ram_write_tracking_start()) {
goto fail;
--
2.25.1
