[Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Thomas Huth]

qemu_savevm_state_iterate() expects the iterators to return 1
when they are done, and 0 if there is still something left to do.
However, ram_save_iterate() does not obey this rule and returns
the number of saved pages instead. This causes a fatal hang with
ppc64 guests when you run QEMU like this (also works with TCG):

 qemu-img create -f qcow2  /tmp/test.qcow2 1M
 qemu-system-ppc64 -nographic -nodefaults -m 256 \
                   -hda /tmp/test.qcow2 -serial mon:stdio

... then switch to the monitor by pressing CTRL-a c and try to
save a snapshot with "savevm test1" for example.

After the first iteration, ram_save_iterate() always returns 0 here,
so that qemu_savevm_state_iterate() hangs in an endless loop and you
can only "kill -9" the QEMU process.
Fix it by using proper return values in ram_save_iterate().

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 migration/ram.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index fb9252d..a1c8089 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1987,7 +1987,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     int ret;
     int i;
     int64_t t0;
-    int pages_sent = 0;
+    int done = 0;
 
     rcu_read_lock();
     if (ram_list.version != last_version) {
@@ -2007,9 +2007,9 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         pages = ram_find_and_save_block(f, false, &bytes_transferred);
         /* no more pages to sent */
         if (pages == 0) {
+            done = 1;
             break;
         }
-        pages_sent += pages;
         acct_info.iterations++;
 
         /* we want to check in the 1st loop, just in case it was the 1st time
@@ -2044,7 +2044,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         return ret;
     }
 
-    return pages_sent;
+    return done;
 }
 
 /* Called with iothread lock */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[David Gibson]

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

On Fri, Nov 04, 2016 at 02:10:17PM +0100, Thomas Huth wrote:
> qemu_savevm_state_iterate() expects the iterators to return 1
> when they are done, and 0 if there is still something left to do.
> However, ram_save_iterate() does not obey this rule and returns
> the number of saved pages instead. This causes a fatal hang with
> ppc64 guests when you run QEMU like this (also works with TCG):
> 
>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>                    -hda /tmp/test.qcow2 -serial mon:stdio
> 
> ... then switch to the monitor by pressing CTRL-a c and try to
> save a snapshot with "savevm test1" for example.
> 
> After the first iteration, ram_save_iterate() always returns 0 here,
> so that qemu_savevm_state_iterate() hangs in an endless loop and you
> can only "kill -9" the QEMU process.
> Fix it by using proper return values in ram_save_iterate().
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Hmm.  I think the change is technically correct, but I'm uneasy with
this approach to the solution.  The whole reason this wasn't caught
earlier is that almost nothing looks at the return value.  Without
changing that I think it's very likely someone will mess this up
again.

I think it would be preferable to change the return type to void to
make it explicit that this function is not directly returning the
"completion" status, but instead that's calculated from the other
progress variables it updates.

> ---
>  migration/ram.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index fb9252d..a1c8089 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1987,7 +1987,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      int ret;
>      int i;
>      int64_t t0;
> -    int pages_sent = 0;
> +    int done = 0;
>  
>      rcu_read_lock();
>      if (ram_list.version != last_version) {
> @@ -2007,9 +2007,9 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          pages = ram_find_and_save_block(f, false, &bytes_transferred);
>          /* no more pages to sent */
>          if (pages == 0) {
> +            done = 1;
>              break;
>          }
> -        pages_sent += pages;
>          acct_info.iterations++;
>  
>          /* we want to check in the 1st loop, just in case it was the 1st time
> @@ -2044,7 +2044,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          return ret;
>      }
>  
> -    return pages_sent;
> +    return done;
>  }
>  
>  /* Called with iothread lock */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Thomas Huth]

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

On 08.11.2016 02:14, David Gibson wrote:
> On Fri, Nov 04, 2016 at 02:10:17PM +0100, Thomas Huth wrote:
>> qemu_savevm_state_iterate() expects the iterators to return 1
>> when they are done, and 0 if there is still something left to do.
>> However, ram_save_iterate() does not obey this rule and returns
>> the number of saved pages instead. This causes a fatal hang with
>> ppc64 guests when you run QEMU like this (also works with TCG):
>>
>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>>                    -hda /tmp/test.qcow2 -serial mon:stdio
>>
>> ... then switch to the monitor by pressing CTRL-a c and try to
>> save a snapshot with "savevm test1" for example.
>>
>> After the first iteration, ram_save_iterate() always returns 0 here,
>> so that qemu_savevm_state_iterate() hangs in an endless loop and you
>> can only "kill -9" the QEMU process.
>> Fix it by using proper return values in ram_save_iterate().
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Hmm.  I think the change is technically correct, but I'm uneasy with
> this approach to the solution.  The whole reason this wasn't caught
> earlier is that almost nothing looks at the return value.  Without
> changing that I think it's very likely someone will mess this up
> again.
> 
> I think it would be preferable to change the return type to void to
> make it explicit that this function is not directly returning the
> "completion" status, but instead that's calculated from the other
> progress variables it updates.

Not sure how such a patch should finally look like. Could you propose a
patch?

Anyway, we're in soft freeze already ... do we still want such a major
change of the logic at this point in time? If not, we should maybe go
with fixing the return type only for 2.8, and do the major change for
2.9 instead?

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Amit Shah]

On (Fri) 04 Nov 2016 [14:10:17], Thomas Huth wrote:
> qemu_savevm_state_iterate() expects the iterators to return 1
> when they are done, and 0 if there is still something left to do.
> However, ram_save_iterate() does not obey this rule and returns
> the number of saved pages instead. This causes a fatal hang with
> ppc64 guests when you run QEMU like this (also works with TCG):

"works with" -- does that mean reproduces with?

>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>                    -hda /tmp/test.qcow2 -serial mon:stdio
> 
> ... then switch to the monitor by pressing CTRL-a c and try to
> save a snapshot with "savevm test1" for example.
> 
> After the first iteration, ram_save_iterate() always returns 0 here,
> so that qemu_savevm_state_iterate() hangs in an endless loop and you
> can only "kill -9" the QEMU process.
> Fix it by using proper return values in ram_save_iterate().
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  migration/ram.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index fb9252d..a1c8089 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1987,7 +1987,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      int ret;
>      int i;
>      int64_t t0;
> -    int pages_sent = 0;
> +    int done = 0;
>  
>      rcu_read_lock();
>      if (ram_list.version != last_version) {
> @@ -2007,9 +2007,9 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          pages = ram_find_and_save_block(f, false, &bytes_transferred);
>          /* no more pages to sent */
>          if (pages == 0) {
> +            done = 1;
>              break;
>          }
> -        pages_sent += pages;
>          acct_info.iterations++;
>  
>          /* we want to check in the 1st loop, just in case it was the 1st time
> @@ -2044,7 +2044,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          return ret;
>      }
>  
> -    return pages_sent;
> +    return done;
>  }

I agree with David, we can just remove the return value.  The first
patch of the series can do that; and this one could become the 2nd
patch.  Should be OK for the soft freeze.

		Amit

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Thomas Huth]

On 09.11.2016 08:18, Amit Shah wrote:
> On (Fri) 04 Nov 2016 [14:10:17], Thomas Huth wrote:
>> qemu_savevm_state_iterate() expects the iterators to return 1
>> when they are done, and 0 if there is still something left to do.
>> However, ram_save_iterate() does not obey this rule and returns
>> the number of saved pages instead. This causes a fatal hang with
>> ppc64 guests when you run QEMU like this (also works with TCG):
> 
> "works with" -- does that mean reproduces with?

Yes, that's what I've meant: You can reproduce it with TCG (e.g. running
on a x86 system), too, there's no need for a real POWER machine with KVM
here.

>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>>                    -hda /tmp/test.qcow2 -serial mon:stdio
>>
>> ... then switch to the monitor by pressing CTRL-a c and try to
>> save a snapshot with "savevm test1" for example.
>>
>> After the first iteration, ram_save_iterate() always returns 0 here,
>> so that qemu_savevm_state_iterate() hangs in an endless loop and you
>> can only "kill -9" the QEMU process.
>> Fix it by using proper return values in ram_save_iterate().
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  migration/ram.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index fb9252d..a1c8089 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1987,7 +1987,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>      int ret;
>>      int i;
>>      int64_t t0;
>> -    int pages_sent = 0;
>> +    int done = 0;
>>  
>>      rcu_read_lock();
>>      if (ram_list.version != last_version) {
>> @@ -2007,9 +2007,9 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>          pages = ram_find_and_save_block(f, false, &bytes_transferred);
>>          /* no more pages to sent */
>>          if (pages == 0) {
>> +            done = 1;
>>              break;
>>          }
>> -        pages_sent += pages;
>>          acct_info.iterations++;
>>  
>>          /* we want to check in the 1st loop, just in case it was the 1st time
>> @@ -2044,7 +2044,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>          return ret;
>>      }
>>  
>> -    return pages_sent;
>> +    return done;
>>  }
> 
> I agree with David, we can just remove the return value.  The first
> patch of the series can do that; and this one could become the 2nd
> patch.  Should be OK for the soft freeze.

Sorry, I still did not quite get it - if I'd change the return type of
ram_save_iterate() and the other iterate functions to "void", how is
qemu_savevm_state_iterate() supposed to know whether all iterators are
done or not? And other iterators also use negative return values to
signal errors - should that then be handled via an "Error **" parameter
instead? ... my gut feeling still says that such a bigger rework (we've
got to touch all iterators for this!) should rather not be done right in
the middle of the freeze period...

 Thomas

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[David Gibson]

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

On Wed, Nov 09, 2016 at 08:46:34AM +0100, Thomas Huth wrote:
> On 09.11.2016 08:18, Amit Shah wrote:
> > On (Fri) 04 Nov 2016 [14:10:17], Thomas Huth wrote:
> >> qemu_savevm_state_iterate() expects the iterators to return 1
> >> when they are done, and 0 if there is still something left to do.
> >> However, ram_save_iterate() does not obey this rule and returns
> >> the number of saved pages instead. This causes a fatal hang with
> >> ppc64 guests when you run QEMU like this (also works with TCG):
> > 
> > "works with" -- does that mean reproduces with?
> 
> Yes, that's what I've meant: You can reproduce it with TCG (e.g. running
> on a x86 system), too, there's no need for a real POWER machine with KVM
> here.
> 
> >>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
> >>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
> >>                    -hda /tmp/test.qcow2 -serial mon:stdio
> >>
> >> ... then switch to the monitor by pressing CTRL-a c and try to
> >> save a snapshot with "savevm test1" for example.
> >>
> >> After the first iteration, ram_save_iterate() always returns 0 here,
> >> so that qemu_savevm_state_iterate() hangs in an endless loop and you
> >> can only "kill -9" the QEMU process.
> >> Fix it by using proper return values in ram_save_iterate().
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  migration/ram.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index fb9252d..a1c8089 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -1987,7 +1987,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> >>      int ret;
> >>      int i;
> >>      int64_t t0;
> >> -    int pages_sent = 0;
> >> +    int done = 0;
> >>  
> >>      rcu_read_lock();
> >>      if (ram_list.version != last_version) {
> >> @@ -2007,9 +2007,9 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> >>          pages = ram_find_and_save_block(f, false, &bytes_transferred);
> >>          /* no more pages to sent */
> >>          if (pages == 0) {
> >> +            done = 1;
> >>              break;
> >>          }
> >> -        pages_sent += pages;
> >>          acct_info.iterations++;
> >>  
> >>          /* we want to check in the 1st loop, just in case it was the 1st time
> >> @@ -2044,7 +2044,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> >>          return ret;
> >>      }
> >>  
> >> -    return pages_sent;
> >> +    return done;
> >>  }
> > 
> > I agree with David, we can just remove the return value.  The first
> > patch of the series can do that; and this one could become the 2nd
> > patch.  Should be OK for the soft freeze.
> 
> Sorry, I still did not quite get it - if I'd change the return type of
> ram_save_iterate() and the other iterate functions to "void", how is
> qemu_savevm_state_iterate() supposed to know whether all iterators are
> done or not?

It doesn't - it's return value is, in turn, mostly ignored by the
caller.

On the migration path we already determine whether to proceed or not
based purely on the separate state_pending callbacks.

For the savevm path, we don't really need the iteration phase at all -
we can jump straight to the completion phase, since downtime is not an
issue.

> And other iterators also use negative return values to
> signal errors

Ah.. that's a good point.  Possibly we should leave in the negative
codes for errors and just remove all positive return values.

> - should that then be handled via an "Error **" parameter
> instead? ... my gut feeling still says that such a bigger rework (we've
> got to touch all iterators for this!) should rather not be done right in
> the middle of the freeze period...

Yeah the errors could - and probably should - be handled with Error **
instead of return codes, but I also wonder if that's too much for soft
freeze.  I guess that's the call of the migration guys.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Dr. David Alan Gilbert]

* Thomas Huth (thuth@redhat.com) wrote:
> On 09.11.2016 08:18, Amit Shah wrote:
> > On (Fri) 04 Nov 2016 [14:10:17], Thomas Huth wrote:
> >> qemu_savevm_state_iterate() expects the iterators to return 1
> >> when they are done, and 0 if there is still something left to do.
> >> However, ram_save_iterate() does not obey this rule and returns
> >> the number of saved pages instead. This causes a fatal hang with
> >> ppc64 guests when you run QEMU like this (also works with TCG):
> > 
> > "works with" -- does that mean reproduces with?
> 
> Yes, that's what I've meant: You can reproduce it with TCG (e.g. running
> on a x86 system), too, there's no need for a real POWER machine with KVM
> here.

How did you trigger it on x86?

Dave

> >>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
> >>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
> >>                    -hda /tmp/test.qcow2 -serial mon:stdio
> >>
> >> ... then switch to the monitor by pressing CTRL-a c and try to
> >> save a snapshot with "savevm test1" for example.
> >>
> >> After the first iteration, ram_save_iterate() always returns 0 here,
> >> so that qemu_savevm_state_iterate() hangs in an endless loop and you
> >> can only "kill -9" the QEMU process.
> >> Fix it by using proper return values in ram_save_iterate().
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  migration/ram.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index fb9252d..a1c8089 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -1987,7 +1987,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> >>      int ret;
> >>      int i;
> >>      int64_t t0;
> >> -    int pages_sent = 0;
> >> +    int done = 0;
> >>  
> >>      rcu_read_lock();
> >>      if (ram_list.version != last_version) {
> >> @@ -2007,9 +2007,9 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> >>          pages = ram_find_and_save_block(f, false, &bytes_transferred);
> >>          /* no more pages to sent */
> >>          if (pages == 0) {
> >> +            done = 1;
> >>              break;
> >>          }
> >> -        pages_sent += pages;
> >>          acct_info.iterations++;
> >>  
> >>          /* we want to check in the 1st loop, just in case it was the 1st time
> >> @@ -2044,7 +2044,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> >>          return ret;
> >>      }
> >>  
> >> -    return pages_sent;
> >> +    return done;
> >>  }
> > 
> > I agree with David, we can just remove the return value.  The first
> > patch of the series can do that; and this one could become the 2nd
> > patch.  Should be OK for the soft freeze.
> 
> Sorry, I still did not quite get it - if I'd change the return type of
> ram_save_iterate() and the other iterate functions to "void", how is
> qemu_savevm_state_iterate() supposed to know whether all iterators are
> done or not? And other iterators also use negative return values to
> signal errors - should that then be handled via an "Error **" parameter
> instead? ... my gut feeling still says that such a bigger rework (we've
> got to touch all iterators for this!) should rather not be done right in
> the middle of the freeze period...
> 
>  Thomas
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Thomas Huth]

On 09.11.2016 16:13, Dr. David Alan Gilbert wrote:
> * Thomas Huth (thuth@redhat.com) wrote:
>> On 09.11.2016 08:18, Amit Shah wrote:
>>> On (Fri) 04 Nov 2016 [14:10:17], Thomas Huth wrote:
>>>> qemu_savevm_state_iterate() expects the iterators to return 1
>>>> when they are done, and 0 if there is still something left to do.
>>>> However, ram_save_iterate() does not obey this rule and returns
>>>> the number of saved pages instead. This causes a fatal hang with
>>>> ppc64 guests when you run QEMU like this (also works with TCG):
>>>
>>> "works with" -- does that mean reproduces with?
>>
>> Yes, that's what I've meant: You can reproduce it with TCG (e.g. running
>> on a x86 system), too, there's no need for a real POWER machine with KVM
>> here.
> 
> How did you trigger it on x86?

As described below - qemu-img + qemu-system-ppc64 + savevm is enough to
trigger it on a x86 host.

> 
>>>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>>>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>>>>                    -hda /tmp/test.qcow2 -serial mon:stdio
>>>>
>>>> ... then switch to the monitor by pressing CTRL-a c and try to
>>>> save a snapshot with "savevm test1" for example.

 Thomas

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Dr. David Alan Gilbert]

* Thomas Huth (thuth@redhat.com) wrote:
> On 09.11.2016 16:13, Dr. David Alan Gilbert wrote:
> > * Thomas Huth (thuth@redhat.com) wrote:
> >> On 09.11.2016 08:18, Amit Shah wrote:
> >>> On (Fri) 04 Nov 2016 [14:10:17], Thomas Huth wrote:
> >>>> qemu_savevm_state_iterate() expects the iterators to return 1
> >>>> when they are done, and 0 if there is still something left to do.
> >>>> However, ram_save_iterate() does not obey this rule and returns
> >>>> the number of saved pages instead. This causes a fatal hang with
> >>>> ppc64 guests when you run QEMU like this (also works with TCG):
> >>>
> >>> "works with" -- does that mean reproduces with?
> >>
> >> Yes, that's what I've meant: You can reproduce it with TCG (e.g. running
> >> on a x86 system), too, there's no need for a real POWER machine with KVM
> >> here.
> > 
> > How did you trigger it on x86?
> 
> As described below - qemu-img + qemu-system-ppc64 + savevm is enough to
> trigger it on a x86 host.

Oh OK; so yes still ppc64 target.

Dave

> > 
> >>>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
> >>>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
> >>>>                    -hda /tmp/test.qcow2 -serial mon:stdio
> >>>>
> >>>> ... then switch to the monitor by pressing CTRL-a c and try to
> >>>> save a snapshot with "savevm test1" for example.
> 
>  Thomas
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> qemu_savevm_state_iterate() expects the iterators to return 1
> when they are done, and 0 if there is still something left to do.
> However, ram_save_iterate() does not obey this rule and returns
> the number of saved pages instead. This causes a fatal hang with
> ppc64 guests when you run QEMU like this (also works with TCG):
>
>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>                    -hda /tmp/test.qcow2 -serial mon:stdio
>
> ... then switch to the monitor by pressing CTRL-a c and try to
> save a snapshot with "savevm test1" for example.
>
> After the first iteration, ram_save_iterate() always returns 0 here,
> so that qemu_savevm_state_iterate() hangs in an endless loop and you
> can only "kill -9" the QEMU process.
> Fix it by using proper return values in ram_save_iterate().
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Applied.

I don't know how we broked this so much.

Thanks, Juan.

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[David Gibson]

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

On Mon, Nov 14, 2016 at 07:34:59PM +0100, Juan Quintela wrote:
> Thomas Huth <thuth@redhat.com> wrote:
> > qemu_savevm_state_iterate() expects the iterators to return 1
> > when they are done, and 0 if there is still something left to do.
> > However, ram_save_iterate() does not obey this rule and returns
> > the number of saved pages instead. This causes a fatal hang with
> > ppc64 guests when you run QEMU like this (also works with TCG):
> >
> >  qemu-img create -f qcow2  /tmp/test.qcow2 1M
> >  qemu-system-ppc64 -nographic -nodefaults -m 256 \
> >                    -hda /tmp/test.qcow2 -serial mon:stdio
> >
> > ... then switch to the monitor by pressing CTRL-a c and try to
> > save a snapshot with "savevm test1" for example.
> >
> > After the first iteration, ram_save_iterate() always returns 0 here,
> > so that qemu_savevm_state_iterate() hangs in an endless loop and you
> > can only "kill -9" the QEMU process.
> > Fix it by using proper return values in ram_save_iterate().
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> Applied.
> 
> I don't know how we broked this so much.

Note that block save iterate has the same bug...

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Thomas Huth]

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

On 17.11.2016 04:45, David Gibson wrote:
> On Mon, Nov 14, 2016 at 07:34:59PM +0100, Juan Quintela wrote:
>> Thomas Huth <thuth@redhat.com> wrote:
>>> qemu_savevm_state_iterate() expects the iterators to return 1
>>> when they are done, and 0 if there is still something left to do.
>>> However, ram_save_iterate() does not obey this rule and returns
>>> the number of saved pages instead. This causes a fatal hang with
>>> ppc64 guests when you run QEMU like this (also works with TCG):
>>>
>>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>>>                    -hda /tmp/test.qcow2 -serial mon:stdio
>>>
>>> ... then switch to the monitor by pressing CTRL-a c and try to
>>> save a snapshot with "savevm test1" for example.
>>>
>>> After the first iteration, ram_save_iterate() always returns 0 here,
>>> so that qemu_savevm_state_iterate() hangs in an endless loop and you
>>> can only "kill -9" the QEMU process.
>>> Fix it by using proper return values in ram_save_iterate().
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>
>> Applied.
>>
>> I don't know how we broked this so much.
> 
> Note that block save iterate has the same bug...

I think you're right. Care to send a patch?

 Thomas




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[Qemu-devel] Is block_save_iterate() dead code? (was: migration: Fix return code of ram_save_iterate() )

[Thomas Huth]

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

On 18.11.2016 09:13, Thomas Huth wrote:
> On 17.11.2016 04:45, David Gibson wrote:
>> On Mon, Nov 14, 2016 at 07:34:59PM +0100, Juan Quintela wrote:
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>> qemu_savevm_state_iterate() expects the iterators to return 1
>>>> when they are done, and 0 if there is still something left to do.
>>>> However, ram_save_iterate() does not obey this rule and returns
>>>> the number of saved pages instead. This causes a fatal hang with
>>>> ppc64 guests when you run QEMU like this (also works with TCG):
>>>>
>>>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>>>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>>>>                    -hda /tmp/test.qcow2 -serial mon:stdio
>>>>
>>>> ... then switch to the monitor by pressing CTRL-a c and try to
>>>> save a snapshot with "savevm test1" for example.
>>>>
>>>> After the first iteration, ram_save_iterate() always returns 0 here,
>>>> so that qemu_savevm_state_iterate() hangs in an endless loop and you
>>>> can only "kill -9" the QEMU process.
>>>> Fix it by using proper return values in ram_save_iterate().
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>
>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>
>>> Applied.
>>>
>>> I don't know how we broked this so much.
>>
>> Note that block save iterate has the same bug...
> 
> I think you're right. Care to send a patch?

Looking at this issue again ... could it be that block_save_iterate() is
currently just dead code?
As far as I can see, the ->save_live_iterate() handlers are only called
from qemu_savevm_state_iterate(), right? And qemu_savevm_state_iterate()
only calls the handlers if se->ops->is_active(se->opaque) returns true.
But block_is_active() seems to only return 0 during savevm, most likely
because qemu_savevm_state() explicitly sets the "blk" and "shared"
MigrationParams to zero.
So to me, it looks like we could also just remove block_save_iterate()
completely ... or did I miss something here?

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] Is block_save_iterate() dead code? (was: migration: Fix return code of ram_save_iterate() )

[Dr. David Alan Gilbert]

* Thomas Huth (thuth@redhat.com) wrote:
> On 18.11.2016 09:13, Thomas Huth wrote:
> > On 17.11.2016 04:45, David Gibson wrote:
> >> On Mon, Nov 14, 2016 at 07:34:59PM +0100, Juan Quintela wrote:
> >>> Thomas Huth <thuth@redhat.com> wrote:
> >>>> qemu_savevm_state_iterate() expects the iterators to return 1
> >>>> when they are done, and 0 if there is still something left to do.
> >>>> However, ram_save_iterate() does not obey this rule and returns
> >>>> the number of saved pages instead. This causes a fatal hang with
> >>>> ppc64 guests when you run QEMU like this (also works with TCG):
> >>>>
> >>>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
> >>>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
> >>>>                    -hda /tmp/test.qcow2 -serial mon:stdio
> >>>>
> >>>> ... then switch to the monitor by pressing CTRL-a c and try to
> >>>> save a snapshot with "savevm test1" for example.
> >>>>
> >>>> After the first iteration, ram_save_iterate() always returns 0 here,
> >>>> so that qemu_savevm_state_iterate() hangs in an endless loop and you
> >>>> can only "kill -9" the QEMU process.
> >>>> Fix it by using proper return values in ram_save_iterate().
> >>>>
> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>
> >>> Reviewed-by: Juan Quintela <quintela@redhat.com>
> >>>
> >>> Applied.
> >>>
> >>> I don't know how we broked this so much.
> >>
> >> Note that block save iterate has the same bug...
> > 
> > I think you're right. Care to send a patch?
> 
> Looking at this issue again ... could it be that block_save_iterate() is
> currently just dead code?
> As far as I can see, the ->save_live_iterate() handlers are only called
> from qemu_savevm_state_iterate(), right? And qemu_savevm_state_iterate()
> only calls the handlers if se->ops->is_active(se->opaque) returns true.
> But block_is_active() seems to only return 0 during savevm, most likely
> because qemu_savevm_state() explicitly sets the "blk" and "shared"
> MigrationParams to zero.
> So to me, it looks like we could also just remove block_save_iterate()
> completely ... or did I miss something here?

Doesn't it get called by migrate -b ?

Dave

>  Thomas
> 
> 



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

Re: [Qemu-devel] Is block_save_iterate() dead code?

[Thomas Huth]

On 16.12.2016 18:03, Dr. David Alan Gilbert wrote:
> * Thomas Huth (thuth@redhat.com) wrote:
>> On 18.11.2016 09:13, Thomas Huth wrote:
>>> On 17.11.2016 04:45, David Gibson wrote:
>>>> On Mon, Nov 14, 2016 at 07:34:59PM +0100, Juan Quintela wrote:
>>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>>> qemu_savevm_state_iterate() expects the iterators to return 1
>>>>>> when they are done, and 0 if there is still something left to do.
>>>>>> However, ram_save_iterate() does not obey this rule and returns
>>>>>> the number of saved pages instead. This causes a fatal hang with
>>>>>> ppc64 guests when you run QEMU like this (also works with TCG):
>>>>>>
>>>>>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>>>>>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>>>>>>                    -hda /tmp/test.qcow2 -serial mon:stdio
>>>>>>
>>>>>> ... then switch to the monitor by pressing CTRL-a c and try to
>>>>>> save a snapshot with "savevm test1" for example.
>>>>>>
>>>>>> After the first iteration, ram_save_iterate() always returns 0 here,
>>>>>> so that qemu_savevm_state_iterate() hangs in an endless loop and you
>>>>>> can only "kill -9" the QEMU process.
>>>>>> Fix it by using proper return values in ram_save_iterate().
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>
>>>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>>>
>>>>> Applied.
>>>>>
>>>>> I don't know how we broked this so much.
>>>>
>>>> Note that block save iterate has the same bug...
>>>
>>> I think you're right. Care to send a patch?
>>
>> Looking at this issue again ... could it be that block_save_iterate() is
>> currently just dead code?
>> As far as I can see, the ->save_live_iterate() handlers are only called
>> from qemu_savevm_state_iterate(), right? And qemu_savevm_state_iterate()
>> only calls the handlers if se->ops->is_active(se->opaque) returns true.
>> But block_is_active() seems to only return 0 during savevm, most likely
>> because qemu_savevm_state() explicitly sets the "blk" and "shared"
>> MigrationParams to zero.
>> So to me, it looks like we could also just remove block_save_iterate()
>> completely ... or did I miss something here?
> 
> Doesn't it get called by migrate -b ?

Ah, right, yes, I somehow missed that ... I probably shouldn't do such
experiments at the end of Friday afternoon ;-)

OK, so it seems that
- block_save_iterate() is not called during savevm at all
  (and thus the bad return code does not matter here)
- migrate -b runs block_save_iterate() but the return code is ignored in
  migration_thread()

So we do not have a real problem here, but I think we should still clean
up the return code of block_save_iterate() to be on the safe side for
the future...

 Thomas

Re: [Qemu-devel] Is block_save_iterate() dead code?

[John Snow]

On 12/19/2016 11:30 AM, Thomas Huth wrote:
> On 16.12.2016 18:03, Dr. David Alan Gilbert wrote:
>> * Thomas Huth (thuth@redhat.com) wrote:
>>> On 18.11.2016 09:13, Thomas Huth wrote:
>>>> On 17.11.2016 04:45, David Gibson wrote:
>>>>> On Mon, Nov 14, 2016 at 07:34:59PM +0100, Juan Quintela wrote:
>>>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>>>> qemu_savevm_state_iterate() expects the iterators to return 1
>>>>>>> when they are done, and 0 if there is still something left to do.
>>>>>>> However, ram_save_iterate() does not obey this rule and returns
>>>>>>> the number of saved pages instead. This causes a fatal hang with
>>>>>>> ppc64 guests when you run QEMU like this (also works with TCG):
>>>>>>>
>>>>>>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>>>>>>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>>>>>>>                    -hda /tmp/test.qcow2 -serial mon:stdio
>>>>>>>
>>>>>>> ... then switch to the monitor by pressing CTRL-a c and try to
>>>>>>> save a snapshot with "savevm test1" for example.
>>>>>>>
>>>>>>> After the first iteration, ram_save_iterate() always returns 0 here,
>>>>>>> so that qemu_savevm_state_iterate() hangs in an endless loop and you
>>>>>>> can only "kill -9" the QEMU process.
>>>>>>> Fix it by using proper return values in ram_save_iterate().
>>>>>>>
>>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>>
>>>>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>>>>
>>>>>> Applied.
>>>>>>
>>>>>> I don't know how we broked this so much.
>>>>>
>>>>> Note that block save iterate has the same bug...
>>>>
>>>> I think you're right. Care to send a patch?
>>>
>>> Looking at this issue again ... could it be that block_save_iterate() is
>>> currently just dead code?
>>> As far as I can see, the ->save_live_iterate() handlers are only called
>>> from qemu_savevm_state_iterate(), right? And qemu_savevm_state_iterate()
>>> only calls the handlers if se->ops->is_active(se->opaque) returns true.
>>> But block_is_active() seems to only return 0 during savevm, most likely
>>> because qemu_savevm_state() explicitly sets the "blk" and "shared"
>>> MigrationParams to zero.
>>> So to me, it looks like we could also just remove block_save_iterate()
>>> completely ... or did I miss something here?
>>
>> Doesn't it get called by migrate -b ?
> 
> Ah, right, yes, I somehow missed that ... I probably shouldn't do such
> experiments at the end of Friday afternoon ;-)
> 
> OK, so it seems that
> - block_save_iterate() is not called during savevm at all
>   (and thus the bad return code does not matter here)
> - migrate -b runs block_save_iterate() but the return code is ignored in
>   migration_thread()
> 
> So we do not have a real problem here, but I think we should still clean
> up the return code of block_save_iterate() to be on the safe side for
> the future...
> 
>  Thomas
> 
> 

If it confused you, it'll confuse someone else. Worth fixing for
consistency's sake alone.

--js