| Message ID | 20210319145249.425189-2-andrey.gruzdev@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Fixes to the 'background-snapshot' code | expand |
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 <andrey.gruzdev@virtuozzo.com> > --- > 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,
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 <andrey.gruzdev@virtuozzo.com> >> --- >> 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.
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 <andrey.gruzdev@virtuozzo.com> > > > --- > > > 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,
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 <andrey.gruzdev@virtuozzo.com> >>>> --- >>>> 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.
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,
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, >
* 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 >
On Wed, Mar 24, 2021 at 05:35:44PM +0000, Dr. David Alan Gilbert wrote: > * 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. 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,
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;
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 <andrey.gruzdev@virtuozzo.com> --- migration/migration.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)