[Qemu-devel] savevm/loadvm

[Qemu-devel] savevm/loadvm

[Alexey Kardashevskiy]

Hi!

I need the community help with savevm/loadvm.

I run QEMU like this:

./qemu-system-ppc64 \
 -drive file=virtimg/fc19_16GB.qcow2 \
 -nodefaults \
 -m "2048" \
 -machine "pseries" \
 -nographic \
 -vga "none" \
 -enable-kvm


The disk image is an 16GB qcow2 image.

Now I start the guest and do "savevm 1" and "loadvm 1" from the qemu
console. Everything works. Then I exit qemu, make sure that the snapshot is
there and run QEMU as above plus "-loadvm 1". It fails with:

qemu-system-ppc64: qcow2: Loading snapshots with different disk size is not
implemented
qemu-system-ppc64: Error -95 while activating snapshot '2' on 'scsi0-hd0'

The check is added by commit 90b277593df873d3a2480f002e2eb5fe1f8e5277
"qcow2: Save disk size in snapshot header".

As I cannot realize the whole idea of the patch, I looked a bit deeper.
This is the check:

int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
{
[...]
    if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
        error_report("qcow2: Loading snapshots with different disk "
            "size is not implemented");
        ret = -ENOTSUP;
        goto fail;
    }


My understanding of the patch was that the disk_size should remain 16GB
(0x4.0000.0000) as it uses bs->total_sectors and never changes it. And
bs->growable is 0 for qcow2 image because it is not really growable. At
least the total_sectors value from the qcow2 file header does not change
between QEMU starts.

However qcow2_save_vmstate() sets bs->growable to 1 for a short time
(commit 178e08a58f40dd5aef2ce774fe0850f5d0e56918 from 2009) and this
triggers a branch in bdrv_co_do_writev() which changes bs->total_sectors.
So when QEMU writes snapshots to the file, the disk_size field of a
snapshot has bigger value (for example 0x4.007b.8180).

And the check above fails. It does not fail if to do "loadvm"
_in_the_same_run_ after "savevm" because QEMU operates with the updated
bs->total_sectors.

What the proper fix would be? Or it is not a bug at all and I should be
using something else for "-loadvm"? Thanks.



-- 
Alexey

Re: [Qemu-devel] savevm/loadvm

[Paolo Bonzini]

Il 08/10/2013 10:40, Alexey Kardashevskiy ha scritto:
> However qcow2_save_vmstate() sets bs->growable to 1 for a short time
> (commit 178e08a58f40dd5aef2ce774fe0850f5d0e56918 from 2009) and this
> triggers a branch in bdrv_co_do_writev() which changes bs->total_sectors.
> So when QEMU writes snapshots to the file, the disk_size field of a
> snapshot has bigger value (for example 0x4.007b.8180).

I think you need to modify qcow2_save_vmstate to save and restore
bs->total_sectors.  Can you test that and if so post the patch?

Paolo

Re: [Qemu-devel] savevm/loadvm

[Kevin Wolf]

Am 08.10.2013 um 11:04 hat Paolo Bonzini geschrieben:
> Il 08/10/2013 10:40, Alexey Kardashevskiy ha scritto:
> > However qcow2_save_vmstate() sets bs->growable to 1 for a short time
> > (commit 178e08a58f40dd5aef2ce774fe0850f5d0e56918 from 2009) and this
> > triggers a branch in bdrv_co_do_writev() which changes bs->total_sectors.
> > So when QEMU writes snapshots to the file, the disk_size field of a
> > snapshot has bigger value (for example 0x4.007b.8180).
> 
> I think you need to modify qcow2_save_vmstate to save and restore
> bs->total_sectors.  Can you test that and if so post the patch?

It's a regression introduced by commit df2a6f29, right?

What you suggest probably works as a quick hack to fix the bug. The VM
state functions in qcow2 are getting uglier and uglier, though. Maybe
they should avoid going through block.c and adding hacks to disable or
revert side effects.

Kevin

Re: [Qemu-devel] savevm/loadvm

[Paolo Bonzini]

Il 08/10/2013 11:23, Kevin Wolf ha scritto:
>> > I think you need to modify qcow2_save_vmstate to save and restore
>> > bs->total_sectors.  Can you test that and if so post the patch?
> It's a regression introduced by commit df2a6f29, right?

Yes, that's what introduced the "if".

> What you suggest probably works as a quick hack to fix the bug. The VM
> state functions in qcow2 are getting uglier and uglier, though. Maybe
> they should avoid going through block.c and adding hacks to disable or
> revert side effects.

Yes, that would work too.

Paolo

Re: [Qemu-devel] savevm/loadvm

[Alexey Kardashevskiy]

On 10/08/2013 08:33 PM, Paolo Bonzini wrote:
> Il 08/10/2013 11:23, Kevin Wolf ha scritto:
>>>> I think you need to modify qcow2_save_vmstate to save and restore
>>>> bs->total_sectors.  Can you test that and if so post the patch?
>> It's a regression introduced by commit df2a6f29, right?
> 
> Yes, that's what introduced the "if".
> 
>> What you suggest probably works as a quick hack to fix the bug. The VM
>> state functions in qcow2 are getting uglier and uglier, though. Maybe
>> they should avoid going through block.c and adding hacks to disable or
>> revert side effects.
> 
> Yes, that would work too.

Sorry for my ignorance (I never ever touched this part of qemu) but how can
you possibly avoid block.c while doing savevm? The qcow2 driver must not
use posix read()/write(), right? So no matter how, all writes end up in
bdrv_co_do_writev() which changes blocks number. Or use
raw_aio_readv()/raw_aio_writev() API directly? Please give some more hints.
Thanks.


-- 
Alexey

Re: [Qemu-devel] savevm/loadvm

[Paolo Bonzini]

Il 09/10/2013 09:15, Alexey Kardashevskiy ha scritto:
> Sorry for my ignorance (I never ever touched this part of qemu) but how can
> you possibly avoid block.c while doing savevm? The qcow2 driver must not
> use posix read()/write(), right? So no matter how, all writes end up in
> bdrv_co_do_writev() which changes blocks number. Or use
> raw_aio_readv()/raw_aio_writev() API directly? Please give some more hints.
> Thanks.

I think Kevin was suggesting using qcow_aio_writev directly, or
something like that.  But it is not trivial, especially because
save_vm_state takes byte offsets instead of sectors.  So for now I'd
still go for the more hacky solution.

Paolo

Re: [Qemu-devel] savevm/loadvm

[Alexey Kardashevskiy]

On 10/09/2013 06:47 PM, Paolo Bonzini wrote:
> Il 09/10/2013 09:15, Alexey Kardashevskiy ha scritto:
>> Sorry for my ignorance (I never ever touched this part of qemu) but how can
>> you possibly avoid block.c while doing savevm? The qcow2 driver must not
>> use posix read()/write(), right? So no matter how, all writes end up in
>> bdrv_co_do_writev() which changes blocks number. Or use
>> raw_aio_readv()/raw_aio_writev() API directly? Please give some more hints.
>> Thanks.
> 
> I think Kevin was suggesting using qcow_aio_writev directly, or
> something like that.  But it is not trivial, especially because
> save_vm_state takes byte offsets instead of sectors.  So for now I'd
> still go for the more hacky solution.

I failed to find qcow_aio_writev() or anything like that. qcow2_co_writev()
uses block.c. And I tried this:

diff --git a/block/qcow2.c b/block/qcow2.c
index 4a9888c..17faf8b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1837,10 +1837,16 @@ static int qcow2_save_vmstate(BlockDriverState *bs,
QEMUIOVector *qiov,
     BDRVQcowState *s = bs->opaque;
     int growable = bs->growable;
     int ret;
+    int64_t total_sectors = bs->total_sectors;

     BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
     bs->growable = 1;
     ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
+    /*
+     * Setting @growable may cause underlying bdrv_co_do_writev()
+     * to increase bs->total_sectors and we do not want this to happen.
+     */
+    bs->total_sectors = total_sectors;
     bs->growable = growable;

     return ret;


It breaks loadvm in a different (weird) way, the error is something like
"ram" or "spapr/htab" (streams registered with register_savevm_live())
chunk cannot be read. Need to debug more...


-- 
Alexey

Re: [Qemu-devel] savevm/loadvm

[Alexey Kardashevskiy]

On 10/10/2013 02:50 PM, Alexey Kardashevskiy wrote:
> On 10/09/2013 06:47 PM, Paolo Bonzini wrote:
>> Il 09/10/2013 09:15, Alexey Kardashevskiy ha scritto:
>>> Sorry for my ignorance (I never ever touched this part of qemu) but how can
>>> you possibly avoid block.c while doing savevm? The qcow2 driver must not
>>> use posix read()/write(), right? So no matter how, all writes end up in
>>> bdrv_co_do_writev() which changes blocks number. Or use
>>> raw_aio_readv()/raw_aio_writev() API directly? Please give some more hints.
>>> Thanks.
>>
>> I think Kevin was suggesting using qcow_aio_writev directly, or
>> something like that.  But it is not trivial, especially because
>> save_vm_state takes byte offsets instead of sectors.  So for now I'd
>> still go for the more hacky solution.
> 
> I failed to find qcow_aio_writev() or anything like that. qcow2_co_writev()
> uses block.c. And I tried this:
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4a9888c..17faf8b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1837,10 +1837,16 @@ static int qcow2_save_vmstate(BlockDriverState *bs,
> QEMUIOVector *qiov,
>      BDRVQcowState *s = bs->opaque;
>      int growable = bs->growable;
>      int ret;
> +    int64_t total_sectors = bs->total_sectors;
> 
>      BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
>      bs->growable = 1;
>      ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
> +    /*
> +     * Setting @growable may cause underlying bdrv_co_do_writev()
> +     * to increase bs->total_sectors and we do not want this to happen.
> +     */
> +    bs->total_sectors = total_sectors;
>      bs->growable = growable;
> 
>      return ret;
> 
> 
> It breaks loadvm in a different (weird) way, the error is something like
> "ram" or "spapr/htab" (streams registered with register_savevm_live())
> chunk cannot be read. Need to debug more...


Just to keep the conversation going :) The patch below helps while the
patch above creates snapshots which cannot be loaded.

And there is no qcow_aio_writev-like API to fix it, what did you mean?

Why not just revert the breaking patch?

Thanks.

diff --git a/savevm.c b/savevm.c
index e0c8aee..aeda0d1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -42,6 +42,7 @@
 #include "qemu/iov.h"
 #include "block/snapshot.h"
 #include "block/qapi.h"
+#include "block/block_int.h"

 #define SELF_ANNOUNCE_ROUNDS 5

@@ -2389,6 +2390,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     qemu_timeval tv;
     struct tm tm;
     const char *name = qdict_get_try_str(qdict, "name");
+    int64_t total_sectors;

     /* Verify if there is a device that doesn't support snapshots and is
writable */
     bs = NULL;
@@ -2442,6 +2444,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }

     /* save the VM state */
+    total_sectors = bs->total_sectors;
     f = qemu_fopen_bdrv(bs, 1);
     if (!f) {
         monitor_printf(mon, "Could not open VM state file\n");
@@ -2450,6 +2453,11 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     ret = qemu_savevm_state(f);
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
+    /*
+     * Setting @growable may cause underlying bdrv_co_do_writev()
+     * to increase bs->total_sectors and we do not want this to happen.
+     */
+    bs->total_sectors = total_sectors;
     if (ret < 0) {
         monitor_printf(mon, "Error %d while writing VM\n", ret);
         goto the_end;




-- 
Alexey

Re: [Qemu-devel] savevm/loadvm

[Alexey Kardashevskiy]

On 10/16/2013 05:51 PM, Alexey Kardashevskiy wrote:
> On 10/10/2013 02:50 PM, Alexey Kardashevskiy wrote:
>> On 10/09/2013 06:47 PM, Paolo Bonzini wrote:
>>> Il 09/10/2013 09:15, Alexey Kardashevskiy ha scritto:
>>>> Sorry for my ignorance (I never ever touched this part of qemu) but how can
>>>> you possibly avoid block.c while doing savevm? The qcow2 driver must not
>>>> use posix read()/write(), right? So no matter how, all writes end up in
>>>> bdrv_co_do_writev() which changes blocks number. Or use
>>>> raw_aio_readv()/raw_aio_writev() API directly? Please give some more hints.
>>>> Thanks.
>>>
>>> I think Kevin was suggesting using qcow_aio_writev directly, or
>>> something like that.  But it is not trivial, especially because
>>> save_vm_state takes byte offsets instead of sectors.  So for now I'd
>>> still go for the more hacky solution.
>>
>> I failed to find qcow_aio_writev() or anything like that. qcow2_co_writev()
>> uses block.c. And I tried this:
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 4a9888c..17faf8b 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1837,10 +1837,16 @@ static int qcow2_save_vmstate(BlockDriverState *bs,
>> QEMUIOVector *qiov,
>>      BDRVQcowState *s = bs->opaque;
>>      int growable = bs->growable;
>>      int ret;
>> +    int64_t total_sectors = bs->total_sectors;
>>
>>      BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
>>      bs->growable = 1;
>>      ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
>> +    /*
>> +     * Setting @growable may cause underlying bdrv_co_do_writev()
>> +     * to increase bs->total_sectors and we do not want this to happen.
>> +     */
>> +    bs->total_sectors = total_sectors;
>>      bs->growable = growable;
>>
>>      return ret;
>>
>>
>> It breaks loadvm in a different (weird) way, the error is something like
>> "ram" or "spapr/htab" (streams registered with register_savevm_live())
>> chunk cannot be read. Need to debug more...
> 
> 
> Just to keep the conversation going :) The patch below helps while the
> patch above creates snapshots which cannot be loaded.
> 
> And there is no qcow_aio_writev-like API to fix it, what did you mean?
> 
> Why not just revert the breaking patch?


Ping? Or it is all fixed now?



> Thanks.
> 
> diff --git a/savevm.c b/savevm.c
> index e0c8aee..aeda0d1 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -42,6 +42,7 @@
>  #include "qemu/iov.h"
>  #include "block/snapshot.h"
>  #include "block/qapi.h"
> +#include "block/block_int.h"
> 
>  #define SELF_ANNOUNCE_ROUNDS 5
> 
> @@ -2389,6 +2390,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      qemu_timeval tv;
>      struct tm tm;
>      const char *name = qdict_get_try_str(qdict, "name");
> +    int64_t total_sectors;
> 
>      /* Verify if there is a device that doesn't support snapshots and is
> writable */
>      bs = NULL;
> @@ -2442,6 +2444,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
> 
>      /* save the VM state */
> +    total_sectors = bs->total_sectors;
>      f = qemu_fopen_bdrv(bs, 1);
>      if (!f) {
>          monitor_printf(mon, "Could not open VM state file\n");
> @@ -2450,6 +2453,11 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      ret = qemu_savevm_state(f);
>      vm_state_size = qemu_ftell(f);
>      qemu_fclose(f);
> +    /*
> +     * Setting @growable may cause underlying bdrv_co_do_writev()
> +     * to increase bs->total_sectors and we do not want this to happen.
> +     */
> +    bs->total_sectors = total_sectors;
>      if (ret < 0) {
>          monitor_printf(mon, "Error %d while writing VM\n", ret);
>          goto the_end;
> 
> 
> 
> 


-- 
Alexey

Re: [Qemu-devel] savevm/loadvm

[Max Reitz]

Hi,

Sorry I'm just now replying to this. I ran into the same issue (and
another one) and it should be fixed by the upstream commits
eedff66f21e542650d895801549ce05ac108278b and
6e13610aa454beba52944e8df6d93158d68ab911. Those have been merged to
master yesterday, so could you re-build qemu from master and try again?


Kind regards,

Max


On 08.10.2013 10:40, Alexey Kardashevskiy wrote:
> Hi!
>
> I need the community help with savevm/loadvm.
>
> I run QEMU like this:
>
> ./qemu-system-ppc64 \
>  -drive file=virtimg/fc19_16GB.qcow2 \
>  -nodefaults \
>  -m "2048" \
>  -machine "pseries" \
>  -nographic \
>  -vga "none" \
>  -enable-kvm
>
>
> The disk image is an 16GB qcow2 image.
>
> Now I start the guest and do "savevm 1" and "loadvm 1" from the qemu
> console. Everything works. Then I exit qemu, make sure that the snapshot is
> there and run QEMU as above plus "-loadvm 1". It fails with:
>
> qemu-system-ppc64: qcow2: Loading snapshots with different disk size is not
> implemented
> qemu-system-ppc64: Error -95 while activating snapshot '2' on 'scsi0-hd0'
>
> The check is added by commit 90b277593df873d3a2480f002e2eb5fe1f8e5277
> "qcow2: Save disk size in snapshot header".
>
> As I cannot realize the whole idea of the patch, I looked a bit deeper.
> This is the check:
>
> int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> {
> [...]
>     if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
>         error_report("qcow2: Loading snapshots with different disk "
>             "size is not implemented");
>         ret = -ENOTSUP;
>         goto fail;
>     }
>
>
> My understanding of the patch was that the disk_size should remain 16GB
> (0x4.0000.0000) as it uses bs->total_sectors and never changes it. And
> bs->growable is 0 for qcow2 image because it is not really growable. At
> least the total_sectors value from the qcow2 file header does not change
> between QEMU starts.
>
> However qcow2_save_vmstate() sets bs->growable to 1 for a short time
> (commit 178e08a58f40dd5aef2ce774fe0850f5d0e56918 from 2009) and this
> triggers a branch in bdrv_co_do_writev() which changes bs->total_sectors.
> So when QEMU writes snapshots to the file, the disk_size field of a
> snapshot has bigger value (for example 0x4.007b.8180).
>
> And the check above fails. It does not fail if to do "loadvm"
> _in_the_same_run_ after "savevm" because QEMU operates with the updated
> bs->total_sectors.
>
> What the proper fix would be? Or it is not a bug at all and I should be
> using something else for "-loadvm"? Thanks.
>
>
>

Re: [Qemu-devel] savevm/loadvm

[Alexey Kardashevskiy]

On 11/02/2013 01:16 AM, Max Reitz wrote:
> Hi,
> 
> Sorry I'm just now replying to this. I ran into the same issue (and
> another one) and it should be fixed by the upstream commits
> eedff66f21e542650d895801549ce05ac108278b and
> 6e13610aa454beba52944e8df6d93158d68ab911. Those have been merged to
> master yesterday, so could you re-build qemu from master and try again?


Yes, this works now, thanks!


> Kind regards,
> 
> Max
> 
> 
> On 08.10.2013 10:40, Alexey Kardashevskiy wrote:
>> Hi!
>>
>> I need the community help with savevm/loadvm.
>>
>> I run QEMU like this:
>>
>> ./qemu-system-ppc64 \
>>  -drive file=virtimg/fc19_16GB.qcow2 \
>>  -nodefaults \
>>  -m "2048" \
>>  -machine "pseries" \
>>  -nographic \
>>  -vga "none" \
>>  -enable-kvm
>>
>>
>> The disk image is an 16GB qcow2 image.
>>
>> Now I start the guest and do "savevm 1" and "loadvm 1" from the qemu
>> console. Everything works. Then I exit qemu, make sure that the snapshot is
>> there and run QEMU as above plus "-loadvm 1". It fails with:
>>
>> qemu-system-ppc64: qcow2: Loading snapshots with different disk size is not
>> implemented
>> qemu-system-ppc64: Error -95 while activating snapshot '2' on 'scsi0-hd0'
>>
>> The check is added by commit 90b277593df873d3a2480f002e2eb5fe1f8e5277
>> "qcow2: Save disk size in snapshot header".
>>
>> As I cannot realize the whole idea of the patch, I looked a bit deeper.
>> This is the check:
>>
>> int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>> {
>> [...]
>>     if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
>>         error_report("qcow2: Loading snapshots with different disk "
>>             "size is not implemented");
>>         ret = -ENOTSUP;
>>         goto fail;
>>     }
>>
>>
>> My understanding of the patch was that the disk_size should remain 16GB
>> (0x4.0000.0000) as it uses bs->total_sectors and never changes it. And
>> bs->growable is 0 for qcow2 image because it is not really growable. At
>> least the total_sectors value from the qcow2 file header does not change
>> between QEMU starts.
>>
>> However qcow2_save_vmstate() sets bs->growable to 1 for a short time
>> (commit 178e08a58f40dd5aef2ce774fe0850f5d0e56918 from 2009) and this
>> triggers a branch in bdrv_co_do_writev() which changes bs->total_sectors.
>> So when QEMU writes snapshots to the file, the disk_size field of a
>> snapshot has bigger value (for example 0x4.007b.8180).
>>
>> And the check above fails. It does not fail if to do "loadvm"
>> _in_the_same_run_ after "savevm" because QEMU operates with the updated
>> bs->total_sectors.
>>
>> What the proper fix would be? Or it is not a bug at all and I should be
>> using something else for "-loadvm"? Thanks.
>>
>>
>>
> 


-- 
Alexey