[Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Liang Li]

If there are file write operations in the guest when doing live
migration, the VM downtime will be much longer than the max_downtime,
this is caused by bdrv_flush_all(), this function is a time consuming
operation if there a lot of data have to be flushed to disk.

By adding bdrv_flush_all() before VM stop, we can reduce the time
consumed by bdrv_flush_all() in vm_stop_force_state, this means the
VM down time can be reduced.

The test shows this optimization can help to reduce the VM downtime
from more than 20 seconds to about 100 milliseconds.

Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 migration/migration.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 2c805f1..fc4735c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -655,6 +655,10 @@ static void *migration_thread(void *opaque)
                 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
                 old_vm_running = runstate_is_running();
 
+                /* do flush here is aimed to shorten the VM downtime,
+                 * bdrv_flush_all is a time consuming operation
+                 * when the guest has done some file writing */
+                bdrv_flush_all();
                 ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
                 if (ret >= 0) {
                     qemu_file_set_rate_limit(s->file, INT64_MAX);
-- 
1.9.1

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Juan Quintela]

Liang Li <liang.z.li@intel.com> wrote:
> If there are file write operations in the guest when doing live
> migration, the VM downtime will be much longer than the max_downtime,
> this is caused by bdrv_flush_all(), this function is a time consuming
> operation if there a lot of data have to be flushed to disk.
>
> By adding bdrv_flush_all() before VM stop, we can reduce the time
> consumed by bdrv_flush_all() in vm_stop_force_state, this means the
> VM down time can be reduced.
>
> The test shows this optimization can help to reduce the VM downtime
> from more than 20 seconds to about 100 milliseconds.
>
> Signed-off-by: Liang Li <liang.z.li@intel.com>


This needs further review/changes on the block layer.

First explanation, why I think this don't fix the full problem.
Whith this patch, we fix the problem where we have a dirty block layer
but basically nothing dirtying the memory on the guest (we are moving
the 20 seconds from max_downtime for the blocklayer flush), to 20
seconds until we have decided that the amount of dirty memory is small
enough to be transferred during max_downtime.  But it is still going to
take 20 seconds to flush the block layer, and during that 20 seconds,
the amount of memory that can be dirty is HUGE.

I think our ouptions are:

- tell the block layer at the beggining of migration
  Hey, we are migrating, could you please start flusing data now, and
  don't get the caches to grow too much, please, pretty please.
  (I left the API to the block layer)
- Add on that point a new function:
  bdrvr_flush_all_start()
  That starts the sending of pages, and we "hope" that by the time that
  we have migrated all memory, they have also finished (so our last
  call to block_flush_all() have less work to do)
- Add another function:
  int bdrv_flush_all_timeout(int timeout)
  that returns if timeout pass, telling if it has migrated all pages or
  timeout has passed.  So we can got back to the iterative stage if it
  has taken too long.

Notice that *normally* bdrv_flush_all() is very fast, the problem is
that sometimes it get really, really slow (NFS decided to go slow, TCP
drop a packet, whatever).

Right now, we don't have an interface to detect that cases and got back
to the iterative stage.

So, I agree whit the diagnosis that there is a problem there, but I
think that the solution is more complex that this.  You helped one load
making a different other worse.  I am not sure which of the two
compromises is better :-(

Makes this sense?

Later, Juan.


> ---
>  migration/migration.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 2c805f1..fc4735c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -655,6 +655,10 @@ static void *migration_thread(void *opaque)
>                  qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>                  old_vm_running = runstate_is_running();
>  
> +                /* do flush here is aimed to shorten the VM downtime,
> +                 * bdrv_flush_all is a time consuming operation
> +                 * when the guest has done some file writing */
> +                bdrv_flush_all();
>                  ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>                  if (ret >= 0) {
>                      qemu_file_set_rate_limit(s->file, INT64_MAX);

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Li, Liang Z]

> This needs further review/changes on the block layer.
> 
> First explanation, why I think this don't fix the full problem.
> Whith this patch, we fix the problem where we have a dirty block layer but
> basically nothing dirtying the memory on the guest (we are moving the 20
> seconds from max_downtime for the blocklayer flush), to 20 seconds until
> we have decided that the amount of dirty memory is small enough to be
> transferred during max_downtime.  But it is still going to take 20 seconds to
> flush the block layer, and during that 20 seconds, the amount of memory that
> can be dirty is HUGE.

It's true.

> I think our ouptions are:
> 
> - tell the block layer at the beggining of migration
>   Hey, we are migrating, could you please start flusing data now, and
>   don't get the caches to grow too much, please, pretty please.
>   (I left the API to the block layer)
> - Add on that point a new function:
>   bdrvr_flush_all_start()
>   That starts the sending of pages, and we "hope" that by the time that
>   we have migrated all memory, they have also finished (so our last
>   call to block_flush_all() have less work to do)
> - Add another function:
>   int bdrv_flush_all_timeout(int timeout)
>   that returns if timeout pass, telling if it has migrated all pages or
>   timeout has passed.  So we can got back to the iterative stage if it
>   has taken too long.
> 
> Notice that *normally* bdrv_flush_all() is very fast, the problem is that
> sometimes it get really, really slow (NFS decided to go slow, TCP drop a
> packet, whatever).
> 
> Right now, we don't have an interface to detect that cases and got back to
> the iterative stage.

How about go back to the iterative stage when detect that the pending_size is larger
Than max_size, like this:

+                /* do flush here is aimed to shorten the VM downtime,
+                 * bdrv_flush_all is a time consuming operation
+                 * when the guest has done some file writing */
+                bdrv_flush_all();
+                pending_size = qemu_savevm_state_pending(s->file, max_size);
+                if (pending_size && pending_size >= max_size) {
+                    qemu_mutex_unlock_iothread();
+                    continue;
+                }
                  ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
                  if (ret >= 0) {
                      qemu_file_set_rate_limit(s->file, INT64_MAX);

and this is quite simple.


> So, I agree whit the diagnosis that there is a problem there, but I think that
> the solution is more complex that this.  You helped one load making a
> different other worse.  I am not sure which of the two compromises is
> better :-(
> 
> Makes this sense?
> 
> Later, Juan.
> 

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Kevin Wolf]

[ Cc: qemu-block ]

Am 18.03.2015 um 04:19 hat Li, Liang Z geschrieben:
> > This needs further review/changes on the block layer.
> > 
> > First explanation, why I think this don't fix the full problem.
> > Whith this patch, we fix the problem where we have a dirty block layer but
> > basically nothing dirtying the memory on the guest (we are moving the 20
> > seconds from max_downtime for the blocklayer flush), to 20 seconds until
> > we have decided that the amount of dirty memory is small enough to be
> > transferred during max_downtime.  But it is still going to take 20 seconds to
> > flush the block layer, and during that 20 seconds, the amount of memory that
> > can be dirty is HUGE.
> 
> It's true.

What kind of cache is it actually that takes 20s to flush here?

Inside of qemu, we don't cache a whole lot. If you use qcow2, we do use
a writeback cache for some metadata that might need to be written back,
but it is small and shouldn't take any significant time.

Then we have the kernel page cache, or for some network protocols caches
in the respective libs. This sounds like the right size for a 20s stall,
but don't we require cache.direct=on for live migration anyway for
coherency, i.e. bypassing any such cache?

Lastly there may be a disk cache, but it's too small either.

> > I think our ouptions are:
> > 
> > - tell the block layer at the beggining of migration
> >   Hey, we are migrating, could you please start flusing data now, and
> >   don't get the caches to grow too much, please, pretty please.
> >   (I left the API to the block layer)
> > - Add on that point a new function:
> >   bdrvr_flush_all_start()
> >   That starts the sending of pages, and we "hope" that by the time that
> >   we have migrated all memory, they have also finished (so our last
> >   call to block_flush_all() have less work to do)
> > - Add another function:
> >   int bdrv_flush_all_timeout(int timeout)
> >   that returns if timeout pass, telling if it has migrated all pages or
> >   timeout has passed.  So we can got back to the iterative stage if it
> >   has taken too long.

The problem is that the block layer really doesn't have an option to
control what is getting synced once the data is cached outside of qemu.
Essentially we can do an fdatasync() or we can leave it, that's the only
choice we have.

Now doing an asynchronous fdatasync() in the background is completely
reasonable in order to reduce the amount of data to be flushed later.
But the patch is doing it while holding both the BQL and the AIOContext
lock of the device, which doesn't feel right. Maybe it should schedule a
BH in the AIOContext instead and flush from there asynchronously.


The other thing is that flushing once doesn't mean that new data isn't
accumulating in the cache, especially if you decide to do the background
flush at the start of the migration.

The obvious way to avoid that would be to switch to a writethrough mode,
so any write request goes directly to the disk. This will, however,
impact performance so heavily that it's probably not a realistic option.

An alternative approach could be to repeat the background flush
periodically, either time based or after every x bytes that are written
to a device. Time based should actually be quite easy to implement.


Once we have the flushing in the background, the migration code can
apply any timeouts it wants. If the timeout is exceeded, the flush
wouldn't be aborted but keep running in the background, but migration
can go back to the iterative state anyway.

> > Notice that *normally* bdrv_flush_all() is very fast, the problem is that
> > sometimes it get really, really slow (NFS decided to go slow, TCP drop a
> > packet, whatever).
> > 
> > Right now, we don't have an interface to detect that cases and got back to
> > the iterative stage.
> 
> How about go back to the iterative stage when detect that the pending_size is larger
> Than max_size, like this:
> 
> +                /* do flush here is aimed to shorten the VM downtime,
> +                 * bdrv_flush_all is a time consuming operation
> +                 * when the guest has done some file writing */
> +                bdrv_flush_all();
> +                pending_size = qemu_savevm_state_pending(s->file, max_size);
> +                if (pending_size && pending_size >= max_size) {
> +                    qemu_mutex_unlock_iothread();
> +                    continue;
> +                }
>                   ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>                   if (ret >= 0) {
>                       qemu_file_set_rate_limit(s->file, INT64_MAX);
> 
> and this is quite simple.

Yes, but it is too simple. If you hold all the locks during
bdrv_flush_all(), your VM will effectively stop as soon as it performs
the next I/O access, so you don't win much. And you still don't have a
timeout for cases where the flush takes really long.

Kevin

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Juan Quintela]

Kevin Wolf <kwolf@redhat.com> wrote:
> [ Cc: qemu-block ]
>
> Am 18.03.2015 um 04:19 hat Li, Liang Z geschrieben:
>> > This needs further review/changes on the block layer.
>> > 
>> > First explanation, why I think this don't fix the full problem.
>> > Whith this patch, we fix the problem where we have a dirty block layer but
>> > basically nothing dirtying the memory on the guest (we are moving the 20
>> > seconds from max_downtime for the blocklayer flush), to 20 seconds until
>> > we have decided that the amount of dirty memory is small enough to be
>> > transferred during max_downtime.  But it is still going to take 20 seconds to
>> > flush the block layer, and during that 20 seconds, the amount of memory that
>> > can be dirty is HUGE.
>> 
>> It's true.
>
> What kind of cache is it actually that takes 20s to flush here?

That is a very good question.   When I meassured this (long, long ago),
testing with the same workload, bdrv_flush_all() could take form
100-300ms (what I expected and I can live with that), to several
seconds, what I can't live with.

Basically (this was around RHEL6 times, whatever upstream were there at
the time), my notes of the time point to:

aio.c:quemu_aio_wait()
        .....
        ret = select(max_fd, &rdfds, &wrfds, NULL, NULL);

notice that we are doing a select without a timeout in the iothread,
bad.

I know that the code has changed a lot on that area, the select() don't
exist anymore.  But this exemplifies the problem, something asks the
block layer to do an operation, and it blocks until it finishes, even if
this time it is taking more than usual for whatever reason.  What I
would really is a way to be able to bdrv_flush_all() to take a timeout
parameter and return an error case that it is taking too long.

> Inside of qemu, we don't cache a whole lot. If you use qcow2, we do use
> a writeback cache for some metadata that might need to be written back,
> but it is small and shouldn't take any significant time.
>
> Then we have the kernel page cache, or for some network protocols caches
> in the respective libs. This sounds like the right size for a 20s stall,
> but don't we require cache.direct=on for live migration anyway for
> coherency, i.e. bypassing any such cache?
>
> Lastly there may be a disk cache, but it's too small either.

>> > I think our ouptions are:
>> > 
>> > - tell the block layer at the beggining of migration
>> >   Hey, we are migrating, could you please start flusing data now, and
>> >   don't get the caches to grow too much, please, pretty please.
>> >   (I left the API to the block layer)
>> > - Add on that point a new function:
>> >   bdrvr_flush_all_start()
>> >   That starts the sending of pages, and we "hope" that by the time that
>> >   we have migrated all memory, they have also finished (so our last
>> >   call to block_flush_all() have less work to do)
>> > - Add another function:
>> >   int bdrv_flush_all_timeout(int timeout)
>> >   that returns if timeout pass, telling if it has migrated all pages or
>> >   timeout has passed.  So we can got back to the iterative stage if it
>> >   has taken too long.
>
> The problem is that the block layer really doesn't have an option to
> control what is getting synced once the data is cached outside of qemu.
> Essentially we can do an fdatasync() or we can leave it, that's the only
> choice we have.

See my explanation, if all that qemu is doing is an fdatasync(), just
spawn a thread that do the fdatasync() and if the thread don't finish in
<timeout> time, just return one error.  You can implement that behaviour
whatever you want.


> Now doing an asynchronous fdatasync() in the background is completely
> reasonable in order to reduce the amount of data to be flushed later.
> But the patch is doing it while holding both the BQL and the AIOContext
> lock of the device, which doesn't feel right. Maybe it should schedule a
> BH in the AIOContext instead and flush from there asynchronously.

Position is wrong, definitelly.  We want to start the asynchronous
fdatasync() at the start of migration, or each X milliseconds.  At this
point, we *think* that we can finish the migration on max_downtime
(basically we are ignoring the time that is going to take to migrate
device state and do the block layer flush, but normally this takes in
the other of 100-200ms, so it don't matter at all).

> The other thing is that flushing once doesn't mean that new data isn't
> accumulating in the cache, especially if you decide to do the background
> flush at the start of the migration.

But "pontentially", we would arrive to this point with less "cached"
data everything on the system.

> The obvious way to avoid that would be to switch to a writethrough mode,
> so any write request goes directly to the disk. This will, however,
> impact performance so heavily that it's probably not a realistic option.
>
> An alternative approach could be to repeat the background flush
> periodically, either time based or after every x bytes that are written
> to a device. Time based should actually be quite easy to implement.

We can do it periodically on the migration thread, if the call is
thread_safe.  We already have a loop there, and "kind" of time control,
what we miss is an interface.

> Once we have the flushing in the background, the migration code can
> apply any timeouts it wants. If the timeout is exceeded, the flush
> wouldn't be aborted but keep running in the background, but migration
> can go back to the iterative state anyway.

Yeap, that is what we really want/need.

>> > Notice that *normally* bdrv_flush_all() is very fast, the problem is that
>> > sometimes it get really, really slow (NFS decided to go slow, TCP drop a
>> > packet, whatever).
>> > 
>> > Right now, we don't have an interface to detect that cases and got back to
>> > the iterative stage.
>> 
>> How about go back to the iterative stage when detect that the pending_size is larger
>> Than max_size, like this:
>> 
>> +                /* do flush here is aimed to shorten the VM downtime,
>> +                 * bdrv_flush_all is a time consuming operation
>> +                 * when the guest has done some file writing */
>> +                bdrv_flush_all();
>> +                pending_size = qemu_savevm_state_pending(s->file, max_size);
>> +                if (pending_size && pending_size >= max_size) {
>> +                    qemu_mutex_unlock_iothread();
>> +                    continue;
>> +                }
>>                   ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>>                   if (ret >= 0) {
>>                       qemu_file_set_rate_limit(s->file, INT64_MAX);
>> 
>> and this is quite simple.
>
> Yes, but it is too simple. If you hold all the locks during
> bdrv_flush_all(), your VM will effectively stop as soon as it performs
> the next I/O access, so you don't win much. And you still don't have a
> timeout for cases where the flush takes really long.

This is probably better than what we had now (basically we are
"meassuring" after bdrv_flush_all how much the amount of dirty memory
has changed, and return to iterative stage if it took too much.  A
timeout would be better anyways.  And an interface te start the
synchronization sooner asynchronously would be also good.

Notice that my understanding is that any proper fix for this is 2.4 material.

Thanks, Juan.

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Paolo Bonzini]

On 18/03/2015 13:36, Juan Quintela wrote:
> I know that the code has changed a lot on that area, the select() don't
> exist anymore.

It is still there in aio_poll():

    ret = qemu_poll_ns((GPollFD *)ctx->pollfds->data,
                         ctx->pollfds->len,
                         blocking ? aio_compute_timeout(ctx) : 0);

where aio_compute_timeout will usually return -1.

Paolo

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Kevin Wolf]

Am 18.03.2015 um 13:36 hat Juan Quintela geschrieben:
> Kevin Wolf <kwolf@redhat.com> wrote:
> > The problem is that the block layer really doesn't have an option to
> > control what is getting synced once the data is cached outside of qemu.
> > Essentially we can do an fdatasync() or we can leave it, that's the only
> > choice we have.
> 
> See my explanation, if all that qemu is doing is an fdatasync(), just
> spawn a thread that do the fdatasync() and if the thread don't finish in
> <timeout> time, just return one error.  You can implement that behaviour
> whatever you want.

Yes, and if you use bdrv_co_flush(), this is one of the worker threads
that raw-posix uses. So no new code to write for that.

> > Now doing an asynchronous fdatasync() in the background is completely
> > reasonable in order to reduce the amount of data to be flushed later.
> > But the patch is doing it while holding both the BQL and the AIOContext
> > lock of the device, which doesn't feel right. Maybe it should schedule a
> > BH in the AIOContext instead and flush from there asynchronously.
> 
> Position is wrong, definitelly.  We want to start the asynchronous
> fdatasync() at the start of migration, or each X milliseconds.  At this
> point, we *think* that we can finish the migration on max_downtime
> (basically we are ignoring the time that is going to take to migrate
> device state and do the block layer flush, but normally this takes in
> the other of 100-200ms, so it don't matter at all).
> 
> > The other thing is that flushing once doesn't mean that new data isn't
> > accumulating in the cache, especially if you decide to do the background
> > flush at the start of the migration.
> 
> But "pontentially", we would arrive to this point with less "cached"
> data everything on the system.
> 
> > The obvious way to avoid that would be to switch to a writethrough mode,
> > so any write request goes directly to the disk. This will, however,
> > impact performance so heavily that it's probably not a realistic option.
> >
> > An alternative approach could be to repeat the background flush
> > periodically, either time based or after every x bytes that are written
> > to a device. Time based should actually be quite easy to implement.
> 
> We can do it periodically on the migration thread, if the call is
> thread_safe.  We already have a loop there, and "kind" of time control,
> what we miss is an interface.

Don't do it from the migration thread. You'd have to take locks and stop
the guest from doing I/O while the flush is running.

Instead, create a coroutine and use a BH to enter it from the main loop
of the AIOContext of the block device (i.e. the qemu main loop thread in
most cases, or the dataplane thread if it exists). Then your flush will
run in the background without blocking anyone else.

Time control in coroutines is as easy as calling co_aio_sleep_ns().

> > Once we have the flushing in the background, the migration code can
> > apply any timeouts it wants. If the timeout is exceeded, the flush
> > wouldn't be aborted but keep running in the background, but migration
> > can go back to the iterative state anyway.
> 
> Yeap, that is what we really want/need.

Cool. I don't think it's very hard to get there.

> Notice that my understanding is that any proper fix for this is 2.4 material.

Yes, I agree.

Kevin

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Li, Liang Z]

> >> > Right now, we don't have an interface to detect that cases and got
> >> > back to the iterative stage.
> >>
> >> How about go back to the iterative stage when detect that the
> >> pending_size is larger Than max_size, like this:
> >>
> >> +                /* do flush here is aimed to shorten the VM downtime,
> >> +                 * bdrv_flush_all is a time consuming operation
> >> +                 * when the guest has done some file writing */
> >> +                bdrv_flush_all();
> >> +                pending_size = qemu_savevm_state_pending(s->file, max_size);
> >> +                if (pending_size && pending_size >= max_size) {
> >> +                    qemu_mutex_unlock_iothread();
> >> +                    continue;
> >> +                }
> >>                   ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> >>                   if (ret >= 0) {
> >>                       qemu_file_set_rate_limit(s->file, INT64_MAX);
> >>
> >> and this is quite simple.
> >
> > Yes, but it is too simple. If you hold all the locks during
> > bdrv_flush_all(), your VM will effectively stop as soon as it performs
> > the next I/O access, so you don't win much. And you still don't have a
> > timeout for cases where the flush takes really long.
> 
> This is probably better than what we had now (basically we are "meassuring"
> after bdrv_flush_all how much the amount of dirty memory has changed,
> and return to iterative stage if it took too much.  A timeout would be better
> anyways.  And an interface te start the synchronization sooner
> asynchronously would be also good.
> 
> Notice that my understanding is that any proper fix for this is 2.4 material.

Then, how to deal with this issue in 2.3, leave it here? or make an incomplete fix like I do above?

Liang

> Thanks, Juan.

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Juan Quintela]

"Li, Liang Z" <liang.z.li@intel.com> wrote:
>> >> > Right now, we don't have an interface to detect that cases and got
>> >> > back to the iterative stage.
>> >>
>> >> How about go back to the iterative stage when detect that the
>> >> pending_size is larger Than max_size, like this:
>> >>
>> >> +                /* do flush here is aimed to shorten the VM downtime,
>> >> +                 * bdrv_flush_all is a time consuming operation
>> >> +                 * when the guest has done some file writing */
>> >> +                bdrv_flush_all();
>> >> +                pending_size = qemu_savevm_state_pending(s->file, max_size);
>> >> +                if (pending_size && pending_size >= max_size) {
>> >> +                    qemu_mutex_unlock_iothread();
>> >> +                    continue;
>> >> +                }
>> >>                   ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>> >>                   if (ret >= 0) {
>> >>                       qemu_file_set_rate_limit(s->file, INT64_MAX);
>> >>
>> >> and this is quite simple.
>> >
>> > Yes, but it is too simple. If you hold all the locks during
>> > bdrv_flush_all(), your VM will effectively stop as soon as it performs
>> > the next I/O access, so you don't win much. And you still don't have a
>> > timeout for cases where the flush takes really long.
>> 
>> This is probably better than what we had now (basically we are "meassuring"
>> after bdrv_flush_all how much the amount of dirty memory has changed,
>> and return to iterative stage if it took too much.  A timeout would be better
>> anyways.  And an interface te start the synchronization sooner
>> asynchronously would be also good.
>> 
>> Notice that my understanding is that any proper fix for this is 2.4 material.
>
> Then, how to deal with this issue in 2.3, leave it here? or make an
> incomplete fix like I do above?

I think it is better to leave it here for 2.3. With a patch like this
one, we improve in one load and we got worse in a different load (depens
a lot in the ratio of dirtying memory vs disk).  I have no data which
load is more common, so I prefer to be conservative so late in the
cycle.  What do you think?

Later, Juan.


>
> Liang
>
>> Thanks, Juan.

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Kevin Wolf]

Am 25.03.2015 um 11:50 hat Juan Quintela geschrieben:
> "Li, Liang Z" <liang.z.li@intel.com> wrote:
> >> >> > Right now, we don't have an interface to detect that cases and got
> >> >> > back to the iterative stage.
> >> >>
> >> >> How about go back to the iterative stage when detect that the
> >> >> pending_size is larger Than max_size, like this:
> >> >>
> >> >> +                /* do flush here is aimed to shorten the VM downtime,
> >> >> +                 * bdrv_flush_all is a time consuming operation
> >> >> +                 * when the guest has done some file writing */
> >> >> +                bdrv_flush_all();
> >> >> +                pending_size = qemu_savevm_state_pending(s->file, max_size);
> >> >> +                if (pending_size && pending_size >= max_size) {
> >> >> +                    qemu_mutex_unlock_iothread();
> >> >> +                    continue;
> >> >> +                }
> >> >>                   ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> >> >>                   if (ret >= 0) {
> >> >>                       qemu_file_set_rate_limit(s->file, INT64_MAX);
> >> >>
> >> >> and this is quite simple.
> >> >
> >> > Yes, but it is too simple. If you hold all the locks during
> >> > bdrv_flush_all(), your VM will effectively stop as soon as it performs
> >> > the next I/O access, so you don't win much. And you still don't have a
> >> > timeout for cases where the flush takes really long.
> >> 
> >> This is probably better than what we had now (basically we are "meassuring"
> >> after bdrv_flush_all how much the amount of dirty memory has changed,
> >> and return to iterative stage if it took too much.  A timeout would be better
> >> anyways.  And an interface te start the synchronization sooner
> >> asynchronously would be also good.
> >> 
> >> Notice that my understanding is that any proper fix for this is 2.4 material.
> >
> > Then, how to deal with this issue in 2.3, leave it here? or make an
> > incomplete fix like I do above?
> 
> I think it is better to leave it here for 2.3. With a patch like this
> one, we improve in one load and we got worse in a different load (depens
> a lot in the ratio of dirtying memory vs disk).  I have no data which
> load is more common, so I prefer to be conservative so late in the
> cycle.  What do you think?

I agree, it's too late in the release cycle for such a change.

Kevin

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Li, Liang Z]

> > > Then, how to deal with this issue in 2.3, leave it here? or make an
> > > incomplete fix like I do above?
> >
> > I think it is better to leave it here for 2.3. With a patch like this
> > one, we improve in one load and we got worse in a different load
> > (depens a lot in the ratio of dirtying memory vs disk).  I have no
> > data which load is more common, so I prefer to be conservative so late
> > in the cycle.  What do you think?
> 
> I agree, it's too late in the release cycle for such a change.
> 
> Kevin

That's OK.

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Li, Liang Z]

> > >> >> > Right now, we don't have an interface to detect that cases and
> > >> >> > got back to the iterative stage.
> > >> >>
> > >> >> How about go back to the iterative stage when detect that the
> > >> >> pending_size is larger Than max_size, like this:
> > >> >>
> > >> >> +                /* do flush here is aimed to shorten the VM downtime,
> > >> >> +                 * bdrv_flush_all is a time consuming operation
> > >> >> +                 * when the guest has done some file writing */
> > >> >> +                bdrv_flush_all();
> > >> >> +                pending_size = qemu_savevm_state_pending(s->file,
> max_size);
> > >> >> +                if (pending_size && pending_size >= max_size) {
> > >> >> +                    qemu_mutex_unlock_iothread();
> > >> >> +                    continue;
> > >> >> +                }
> > >> >>                   ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> > >> >>                   if (ret >= 0) {
> > >> >>                       qemu_file_set_rate_limit(s->file,
> > >> >> INT64_MAX);
> > >> >>
> > >> >> and this is quite simple.
> > >> >
> > >> > Yes, but it is too simple. If you hold all the locks during
> > >> > bdrv_flush_all(), your VM will effectively stop as soon as it
> > >> > performs the next I/O access, so you don't win much. And you
> > >> > still don't have a timeout for cases where the flush takes really long.
> > >>
> > >> This is probably better than what we had now (basically we are
> "meassuring"
> > >> after bdrv_flush_all how much the amount of dirty memory has
> > >> changed, and return to iterative stage if it took too much.  A
> > >> timeout would be better anyways.  And an interface te start the
> > >> synchronization sooner asynchronously would be also good.
> > >>
> > >> Notice that my understanding is that any proper fix for this is 2.4
> material.
> > >
> > > Then, how to deal with this issue in 2.3, leave it here? or make an
> > > incomplete fix like I do above?
> >
> > I think it is better to leave it here for 2.3. With a patch like this
> > one, we improve in one load and we got worse in a different load
> > (depens a lot in the ratio of dirtying memory vs disk).  I have no
> > data which load is more common, so I prefer to be conservative so late
> > in the cycle.  What do you think?
> 
> I agree, it's too late in the release cycle for such a change.
> 
> Kevin

Hi Juan & Kevin,

I have not found the related patches to fix the issue which lead to long VM downtime,  how is it going?

Liang

Re: [Qemu-devel] [Qemu-block] [PATCH] migration: flush the bdrv before stopping VM

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2948 bytes --]

On Wed, Jun 24, 2015 at 11:08:43AM +0000, Li, Liang Z wrote:
> > > >> >> > Right now, we don't have an interface to detect that cases and
> > > >> >> > got back to the iterative stage.
> > > >> >>
> > > >> >> How about go back to the iterative stage when detect that the
> > > >> >> pending_size is larger Than max_size, like this:
> > > >> >>
> > > >> >> +                /* do flush here is aimed to shorten the VM downtime,
> > > >> >> +                 * bdrv_flush_all is a time consuming operation
> > > >> >> +                 * when the guest has done some file writing */
> > > >> >> +                bdrv_flush_all();
> > > >> >> +                pending_size = qemu_savevm_state_pending(s->file,
> > max_size);
> > > >> >> +                if (pending_size && pending_size >= max_size) {
> > > >> >> +                    qemu_mutex_unlock_iothread();
> > > >> >> +                    continue;
> > > >> >> +                }
> > > >> >>                   ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> > > >> >>                   if (ret >= 0) {
> > > >> >>                       qemu_file_set_rate_limit(s->file,
> > > >> >> INT64_MAX);
> > > >> >>
> > > >> >> and this is quite simple.
> > > >> >
> > > >> > Yes, but it is too simple. If you hold all the locks during
> > > >> > bdrv_flush_all(), your VM will effectively stop as soon as it
> > > >> > performs the next I/O access, so you don't win much. And you
> > > >> > still don't have a timeout for cases where the flush takes really long.
> > > >>
> > > >> This is probably better than what we had now (basically we are
> > "meassuring"
> > > >> after bdrv_flush_all how much the amount of dirty memory has
> > > >> changed, and return to iterative stage if it took too much.  A
> > > >> timeout would be better anyways.  And an interface te start the
> > > >> synchronization sooner asynchronously would be also good.
> > > >>
> > > >> Notice that my understanding is that any proper fix for this is 2.4
> > material.
> > > >
> > > > Then, how to deal with this issue in 2.3, leave it here? or make an
> > > > incomplete fix like I do above?
> > >
> > > I think it is better to leave it here for 2.3. With a patch like this
> > > one, we improve in one load and we got worse in a different load
> > > (depens a lot in the ratio of dirtying memory vs disk).  I have no
> > > data which load is more common, so I prefer to be conservative so late
> > > in the cycle.  What do you think?
> > 
> > I agree, it's too late in the release cycle for such a change.
> > 
> > Kevin
> 
> Hi Juan & Kevin,
> 
> I have not found the related patches to fix the issue which lead to long VM downtime,  how is it going?

Kevin is on vacation and QEMU is currently in 2.4 soft freeze.  Unless
patches have been posted/merged that I'm not aware of, it is unlikely
that anything will happen before QEMU 2.4 is released.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Li, Liang Z]

> > > First explanation, why I think this don't fix the full problem.
> > > Whith this patch, we fix the problem where we have a dirty block
> > > layer but basically nothing dirtying the memory on the guest (we are
> > > moving the 20 seconds from max_downtime for the blocklayer flush),
> > > to 20 seconds until we have decided that the amount of dirty memory
> > > is small enough to be transferred during max_downtime.  But it is
> > > still going to take 20 seconds to flush the block layer, and during
> > > that 20 seconds, the amount of memory that can be dirty is HUGE.
> >
> > It's true.
> 
> What kind of cache is it actually that takes 20s to flush here?
> 

I run a script in the guest which do a dd operation,  like this:

#!/bin/sh
for i in {1..1000000}
do
	time dd if=/dev/zero of=/time.bdf bs=4k count=200000
	rm /time.bdf
done

It's an extreme  case.

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Dr. David Alan Gilbert]

* Li, Liang Z (liang.z.li@intel.com) wrote:
> > > > First explanation, why I think this don't fix the full problem.
> > > > Whith this patch, we fix the problem where we have a dirty block
> > > > layer but basically nothing dirtying the memory on the guest (we are
> > > > moving the 20 seconds from max_downtime for the blocklayer flush),
> > > > to 20 seconds until we have decided that the amount of dirty memory
> > > > is small enough to be transferred during max_downtime.  But it is
> > > > still going to take 20 seconds to flush the block layer, and during
> > > > that 20 seconds, the amount of memory that can be dirty is HUGE.
> > >
> > > It's true.
> > 
> > What kind of cache is it actually that takes 20s to flush here?
> > 
> 
> I run a script in the guest which do a dd operation,  like this:
> 
> #!/bin/sh
> for i in {1..1000000}
> do
> 	time dd if=/dev/zero of=/time.bdf bs=4k count=200000
> 	rm /time.bdf
> done
> 
> It's an extreme  case.

With what qemu options for the device, and what was your device
backed by?

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Li, Liang Z]

> * Li, Liang Z (liang.z.li@intel.com) wrote:
> > > > > First explanation, why I think this don't fix the full problem.
> > > > > Whith this patch, we fix the problem where we have a dirty block
> > > > > layer but basically nothing dirtying the memory on the guest (we
> > > > > are moving the 20 seconds from max_downtime for the blocklayer
> > > > > flush), to 20 seconds until we have decided that the amount of
> > > > > dirty memory is small enough to be transferred during
> > > > > max_downtime.  But it is still going to take 20 seconds to flush
> > > > > the block layer, and during that 20 seconds, the amount of memory
> that can be dirty is HUGE.
> > > >
> > > > It's true.
> > >
> > > What kind of cache is it actually that takes 20s to flush here?
> > >
> >
> > I run a script in the guest which do a dd operation,  like this:
> >
> > #!/bin/sh
> > for i in {1..1000000}
> > do
> > 	time dd if=/dev/zero of=/time.bdf bs=4k count=200000
> > 	rm /time.bdf
> > done
> >
> > It's an extreme  case.
> 
> With what qemu options for the device, and what was your device backed by?

Very simple:
./qemu-system-x86_64 -enable-kvm -smp 4 -m 4096  -net none rhel6u5.img -monitor stdio

And it's a local migration.  I will do the test between two physical machines later.


Liang

Re: [Qemu-devel] [PATCH] migration: flush the bdrv before stopping VM

[Dr. David Alan Gilbert]

* Li, Liang Z (liang.z.li@intel.com) wrote:
> > * Li, Liang Z (liang.z.li@intel.com) wrote:
> > > > > > First explanation, why I think this don't fix the full problem.
> > > > > > Whith this patch, we fix the problem where we have a dirty block
> > > > > > layer but basically nothing dirtying the memory on the guest (we
> > > > > > are moving the 20 seconds from max_downtime for the blocklayer
> > > > > > flush), to 20 seconds until we have decided that the amount of
> > > > > > dirty memory is small enough to be transferred during
> > > > > > max_downtime.  But it is still going to take 20 seconds to flush
> > > > > > the block layer, and during that 20 seconds, the amount of memory
> > that can be dirty is HUGE.
> > > > >
> > > > > It's true.
> > > >
> > > > What kind of cache is it actually that takes 20s to flush here?
> > > >
> > >
> > > I run a script in the guest which do a dd operation,  like this:
> > >
> > > #!/bin/sh
> > > for i in {1..1000000}
> > > do
> > > 	time dd if=/dev/zero of=/time.bdf bs=4k count=200000
> > > 	rm /time.bdf
> > > done
> > >
> > > It's an extreme  case.
> > 
> > With what qemu options for the device, and what was your device backed by?
> 
> Very simple:
> ./qemu-system-x86_64 -enable-kvm -smp 4 -m 4096  -net none rhel6u5.img -monitor stdio
> 
> And it's a local migration.  I will do the test between two physical machines later.

OK, but for shared storage you would have to add cache=none (or something like that),
so that would change the behaviour anyway.

Dave
> 
> 
> Liang
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK