Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
On 01/12/16 18:40, Markus Armbruster wrote:
> Kevin Wolf writes:
>
>> Am 12.01.2016 um 17:35 hat Denis V. Lunev geschrieben:
>>> On 01/12/2016 06:47 PM, Denis V. Lunev wrote:
On 01/12/2016 06:20 PM, Kevin Wolf wrote:
> Am 12.01.2016 um 15:59 hat Paolo Bonzini geschrieben:
>>
>> On 12/01/2016 15:16, Kevin Wolf wrote:
Thus we should avoid selection of "pflash" drives for VM
state saving.
For now "pflash" is read-write raw image as it configured by libvirt.
Thus there are no such images in the field and we could
safely disable
ability to save state to those images inside QEMU.
>>> This is obviously broken. If you write to the pflash, then it needs to
>>> be snapshotted in order to keep a consistent state.
>>>
>>> If you want to avoid snapshotting the image, make it read-only and it
>>> will be skipped even today.
>> Sort of. The point of having flash is to _not_ make it read-only, so
>> that is not a solution.
>>
>> Flash is already being snapshotted as part of saving RAM state. In
>> fact, for this reason the device (at least the one used with OVMF; I
>> haven't checked other pflash devices) can simply save it back to disk
>> on the migration destination, without the need to use "migrate -b" or
>> shared storage.
>> [...]
>> I don't like very much using IF_PFLASH this way, which is why I hadn't
>> replied to the patch so far---I hadn't made up my mind about *what* to
>> suggest instead, or whether to just accept it. However, it does work.
>>
>> Perhaps a separate "I know what I am doing" skip-snapshot option? Or
>> a device callback saying "not snapshotting this is fine"?
> Boy, is this ugly...
>
> What do you do with disk-only snapshots? The recovery only works as long
> as you have VM state.
>
> Kevin
actually I am in a bit of trouble :(
I understand that this is ugly, but I would like to make working
'virsh snapshot' for OVFM VMs. This is necessary for us to make
a release.
Currently libvirt guys generate XML in the following way:
hvm
>>> type='pflash'>/usr/share/OVMF/OVMF_CODE_new.fd
/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd
This results in:
qemu -drive
file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
\
-drive
file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd,if=pflash,format=raw,unit=1
This obviously can not pass check in bdrv_all_can_snapshot()
as 'pflash' is RW and raw, i.e. can not be snapshoted.
They have discussed the switch to the following command line:
qemu -drive
file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
\
-drive
file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd.qcow2,if=pflash,format=qcow2,unit=1
and say that in this case VM state could fall into PFLASH
drive which is should not be big as the location of the
file is different. This means that I am doomed here.
Either we should force libvirt people to forget about their
opinion that pflash should be small which I am unable to
do or I should invent a way to ban VM state saving into
pflash.
OK. There are 2 options.
1) Ban pflash as it was done.
2) Add 'no-vmstate' flag to -drive (invented just now).
>>> something like this:
>>>
>>> diff --git a/block.c b/block.c
>>> index 3e1877d..8900589 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -881,6 +881,11 @@ static QemuOptsList bdrv_runtime_opts = {
>>> .help = "Block driver to use for the node",
>>> },
>>> {
>>> +.name = "novmstate",
>>> +.type = QEMU_OPT_BOOL,
>>> +.help = "Ignore for selecting to save VM state",
>>> +},
>>> +{
>>> .name = BDRV_OPT_CACHE_WB,
>>> .type = QEMU_OPT_BOOL,
>>> .help = "Enable writeback mode",
>>> @@ -957,6 +962,7 @@ static int bdrv_open_common(BlockDriverState
>>> *bs, BdrvChild *file,
>>> bs->request_alignment = 512;
>>> bs->zero_beyond_eof = true;
>>> bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
>>> +bs->disable_vmstate_save = qemu_opt_get_bool(opts, "novmstate", false);
>>>
>>> if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
>>> error_setg(errp,
>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>> index 2d86b88..33cdd86 100644
>>> --- a/block/snapshot.c
>>> +++ b/block/snapshot.c
>>> @@ -483,6 +483,10 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
>>> while (not_found && (bs = bdrv_next(bs))) {
>>> AioContext *ctx = bdrv_get_aio_context(bs);
>>>
>>> +if (bs->disable_vmstate_save) {
>>> +continue;
>>> +}
>>> +
>>> aio_context_acqui
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
On 01/12/16 18:50, Kevin Wolf wrote:
> Am 12.01.2016 um 18:40 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>>> Am 12.01.2016 um 17:35 hat Denis V. Lunev geschrieben:
On 01/12/2016 06:47 PM, Denis V. Lunev wrote:
> On 01/12/2016 06:20 PM, Kevin Wolf wrote:
>> Am 12.01.2016 um 15:59 hat Paolo Bonzini geschrieben:
>>>
>>> On 12/01/2016 15:16, Kevin Wolf wrote:
> Thus we should avoid selection of "pflash" drives for VM
> state saving.
>
> For now "pflash" is read-write raw image as it configured by libvirt.
> Thus there are no such images in the field and we could
> safely disable
> ability to save state to those images inside QEMU.
This is obviously broken. If you write to the pflash, then it needs to
be snapshotted in order to keep a consistent state.
If you want to avoid snapshotting the image, make it read-only and it
will be skipped even today.
>>> Sort of. The point of having flash is to _not_ make it read-only, so
>>> that is not a solution.
>>>
>>> Flash is already being snapshotted as part of saving RAM state. In
>>> fact, for this reason the device (at least the one used with OVMF; I
>>> haven't checked other pflash devices) can simply save it back to disk
>>> on the migration destination, without the need to use "migrate -b" or
>>> shared storage.
>>> [...]
>>> I don't like very much using IF_PFLASH this way, which is why I hadn't
>>> replied to the patch so far---I hadn't made up my mind about *what* to
>>> suggest instead, or whether to just accept it. However, it does work.
>>>
>>> Perhaps a separate "I know what I am doing" skip-snapshot option? Or
>>> a device callback saying "not snapshotting this is fine"?
>> Boy, is this ugly...
>>
>> What do you do with disk-only snapshots? The recovery only works as long
>> as you have VM state.
>>
>> Kevin
> actually I am in a bit of trouble :(
>
> I understand that this is ugly, but I would like to make working
> 'virsh snapshot' for OVFM VMs. This is necessary for us to make
> a release.
>
> Currently libvirt guys generate XML in the following way:
>
>
>hvm
> type='pflash'>/usr/share/OVMF/OVMF_CODE_new.fd
> /var/lib/libvirt/qemu/nvram/f20efi_VARS.fd
>
>
> This results in:
>
> qemu -drive
> file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
> \
> -drive
> file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd,if=pflash,format=raw,unit=1
>
> This obviously can not pass check in bdrv_all_can_snapshot()
> as 'pflash' is RW and raw, i.e. can not be snapshoted.
>
> They have discussed the switch to the following command line:
>
> qemu -drive
> file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
> \
> -drive
> file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd.qcow2,if=pflash,format=qcow2,unit=1
>
> and say that in this case VM state could fall into PFLASH
> drive which is should not be big as the location of the
> file is different. This means that I am doomed here.
>
> Either we should force libvirt people to forget about their
> opinion that pflash should be small which I am unable to
> do or I should invent a way to ban VM state saving into
> pflash.
>
> OK. There are 2 options.
>
> 1) Ban pflash as it was done.
> 2) Add 'no-vmstate' flag to -drive (invented just now).
>
something like this:
diff --git a/block.c b/block.c
index 3e1877d..8900589 100644
--- a/block.c
+++ b/block.c
@@ -881,6 +881,11 @@ static QemuOptsList bdrv_runtime_opts = {
.help = "Block driver to use for the node",
},
{
+.name = "novmstate",
+.type = QEMU_OPT_BOOL,
+.help = "Ignore for selecting to save VM state",
+},
+{
.name = BDRV_OPT_CACHE_WB,
.type = QEMU_OPT_BOOL,
.help = "Enable writeback mode",
@@ -957,6 +962,7 @@ static int bdrv_open_common(BlockDriverState
*bs, BdrvChild *file,
bs->request_alignment = 512;
bs->zero_beyond_eof = true;
bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
+bs->disable_vmstate_save = qemu_opt_get_bool(opts, "novmstate",
false);
if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
error_setg(errp,
diff --git a/block/snapshot.c b/block/snapshot.c
index 2d86b88..33cdd86 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -483,6 +483,10 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
while (not_found && (bs = bdrv_next(bs))) {
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
Kevin Wolf writes:
> Am 12.01.2016 um 18:40 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>> > Am 12.01.2016 um 17:35 hat Denis V. Lunev geschrieben:
>> >> On 01/12/2016 06:47 PM, Denis V. Lunev wrote:
>> >> >On 01/12/2016 06:20 PM, Kevin Wolf wrote:
>> >> >>Am 12.01.2016 um 15:59 hat Paolo Bonzini geschrieben:
>> >> >>>
>> >> >>>On 12/01/2016 15:16, Kevin Wolf wrote:
>> >> >Thus we should avoid selection of "pflash" drives for VM
>> >> >state saving.
>> >> >
>> >> >For now "pflash" is read-write raw image as it configured by libvirt.
>> >> >Thus there are no such images in the field and we could
>> >> >safely disable
>> >> >ability to save state to those images inside QEMU.
>> >> This is obviously broken. If you write to the pflash, then it needs to
>> >> be snapshotted in order to keep a consistent state.
>> >>
>> >> If you want to avoid snapshotting the image, make it read-only and it
>> >> will be skipped even today.
>> >> >>>Sort of. The point of having flash is to _not_ make it read-only, so
>> >> >>>that is not a solution.
>> >> >>>
>> >> >>>Flash is already being snapshotted as part of saving RAM state. In
>> >> >>>fact, for this reason the device (at least the one used with OVMF; I
>> >> >>>haven't checked other pflash devices) can simply save it back to disk
>> >> >>>on the migration destination, without the need to use "migrate -b" or
>> >> >>>shared storage.
>> >> >>>[...]
>> >> >>>I don't like very much using IF_PFLASH this way, which is why I hadn't
>> >> >>>replied to the patch so far---I hadn't made up my mind about *what* to
>> >> >>>suggest instead, or whether to just accept it. However, it does work.
>> >> >>>
>> >> >>>Perhaps a separate "I know what I am doing" skip-snapshot option? Or
>> >> >>>a device callback saying "not snapshotting this is fine"?
>> >> >>Boy, is this ugly...
>> >> >>
>> >> >>What do you do with disk-only snapshots? The recovery only works as long
>> >> >>as you have VM state.
>> >> >>
>> >> >>Kevin
>> >> >actually I am in a bit of trouble :(
>> >> >
>> >> >I understand that this is ugly, but I would like to make working
>> >> >'virsh snapshot' for OVFM VMs. This is necessary for us to make
>> >> >a release.
>> >> >
>> >> >Currently libvirt guys generate XML in the following way:
>> >> >
>> >> >
>> >> >hvm
>> >> >> >> >type='pflash'>/usr/share/OVMF/OVMF_CODE_new.fd
>> >> >/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd
>> >> >
>> >> >
>> >> >This results in:
>> >> >
>> >> >qemu -drive
>> >> >file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
>> >> >\
>> >> > -drive
>> >> > file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd,if=pflash,format=raw,unit=1
>> >> >
>> >> >This obviously can not pass check in bdrv_all_can_snapshot()
>> >> >as 'pflash' is RW and raw, i.e. can not be snapshoted.
>> >> >
>> >> >They have discussed the switch to the following command line:
>> >> >
>> >> >qemu -drive
>> >> >file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
>> >> >\
>> >> > -drive
>> >> > file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd.qcow2,if=pflash,format=qcow2,unit=1
>> >> >
>> >> >and say that in this case VM state could fall into PFLASH
>> >> >drive which is should not be big as the location of the
>> >> >file is different. This means that I am doomed here.
>> >> >
>> >> >Either we should force libvirt people to forget about their
>> >> >opinion that pflash should be small which I am unable to
>> >> >do or I should invent a way to ban VM state saving into
>> >> >pflash.
>> >> >
>> >> >OK. There are 2 options.
>> >> >
>> >> >1) Ban pflash as it was done.
>> >> >2) Add 'no-vmstate' flag to -drive (invented just now).
>> >> >
>> >> something like this:
>> >>
>> >> diff --git a/block.c b/block.c
>> >> index 3e1877d..8900589 100644
>> >> --- a/block.c
>> >> +++ b/block.c
>> >> @@ -881,6 +881,11 @@ static QemuOptsList bdrv_runtime_opts = {
>> >> .help = "Block driver to use for the node",
>> >> },
>> >> {
>> >> +.name = "novmstate",
>> >> +.type = QEMU_OPT_BOOL,
>> >> +.help = "Ignore for selecting to save VM state",
>> >> +},
>> >> +{
>> >> .name = BDRV_OPT_CACHE_WB,
>> >> .type = QEMU_OPT_BOOL,
>> >> .help = "Enable writeback mode",
>> >> @@ -957,6 +962,7 @@ static int bdrv_open_common(BlockDriverState
>> >> *bs, BdrvChild *file,
>> >> bs->request_alignment = 512;
>> >> bs->zero_beyond_eof = true;
>> >> bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
>> >> +bs->disable_vmstate_save = qemu_opt_get_bool(opts, "novmstate",
>> >> false);
>> >>
>> >> if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
>> >> error_setg(errp,
>> >> diff --git a/block/snapshot.c b/block/snapshot.c
>> >> index 2d86b88..33cdd86 100644
>> >> --- a/block/snapshot.c
>> >> +++ b/block/snapshot.c
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
On 01/12/2016 08:50 PM, Kevin Wolf wrote:
Am 12.01.2016 um 18:40 hat Markus Armbruster geschrieben:
Kevin Wolf writes:
Am 12.01.2016 um 17:35 hat Denis V. Lunev geschrieben:
On 01/12/2016 06:47 PM, Denis V. Lunev wrote:
On 01/12/2016 06:20 PM, Kevin Wolf wrote:
Am 12.01.2016 um 15:59 hat Paolo Bonzini geschrieben:
On 12/01/2016 15:16, Kevin Wolf wrote:
Thus we should avoid selection of "pflash" drives for VM
state saving.
For now "pflash" is read-write raw image as it configured by libvirt.
Thus there are no such images in the field and we could
safely disable
ability to save state to those images inside QEMU.
This is obviously broken. If you write to the pflash, then it needs to
be snapshotted in order to keep a consistent state.
If you want to avoid snapshotting the image, make it read-only and it
will be skipped even today.
Sort of. The point of having flash is to _not_ make it read-only, so
that is not a solution.
Flash is already being snapshotted as part of saving RAM state. In
fact, for this reason the device (at least the one used with OVMF; I
haven't checked other pflash devices) can simply save it back to disk
on the migration destination, without the need to use "migrate -b" or
shared storage.
[...]
I don't like very much using IF_PFLASH this way, which is why I hadn't
replied to the patch so far---I hadn't made up my mind about *what* to
suggest instead, or whether to just accept it. However, it does work.
Perhaps a separate "I know what I am doing" skip-snapshot option? Or
a device callback saying "not snapshotting this is fine"?
Boy, is this ugly...
What do you do with disk-only snapshots? The recovery only works as long
as you have VM state.
Kevin
actually I am in a bit of trouble :(
I understand that this is ugly, but I would like to make working
'virsh snapshot' for OVFM VMs. This is necessary for us to make
a release.
Currently libvirt guys generate XML in the following way:
hvm
/usr/share/OVMF/OVMF_CODE_new.fd
/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd
This results in:
qemu -drive
file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
\
-drive
file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd,if=pflash,format=raw,unit=1
This obviously can not pass check in bdrv_all_can_snapshot()
as 'pflash' is RW and raw, i.e. can not be snapshoted.
They have discussed the switch to the following command line:
qemu -drive
file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
\
-drive
file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd.qcow2,if=pflash,format=qcow2,unit=1
and say that in this case VM state could fall into PFLASH
drive which is should not be big as the location of the
file is different. This means that I am doomed here.
Either we should force libvirt people to forget about their
opinion that pflash should be small which I am unable to
do or I should invent a way to ban VM state saving into
pflash.
OK. There are 2 options.
1) Ban pflash as it was done.
2) Add 'no-vmstate' flag to -drive (invented just now).
something like this:
diff --git a/block.c b/block.c
index 3e1877d..8900589 100644
--- a/block.c
+++ b/block.c
@@ -881,6 +881,11 @@ static QemuOptsList bdrv_runtime_opts = {
.help = "Block driver to use for the node",
},
{
+.name = "novmstate",
+.type = QEMU_OPT_BOOL,
+.help = "Ignore for selecting to save VM state",
+},
+{
.name = BDRV_OPT_CACHE_WB,
.type = QEMU_OPT_BOOL,
.help = "Enable writeback mode",
@@ -957,6 +962,7 @@ static int bdrv_open_common(BlockDriverState
*bs, BdrvChild *file,
bs->request_alignment = 512;
bs->zero_beyond_eof = true;
bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
+bs->disable_vmstate_save = qemu_opt_get_bool(opts, "novmstate", false);
if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
error_setg(errp,
diff --git a/block/snapshot.c b/block/snapshot.c
index 2d86b88..33cdd86 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -483,6 +483,10 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
while (not_found && (bs = bdrv_next(bs))) {
AioContext *ctx = bdrv_get_aio_context(bs);
+if (bs->disable_vmstate_save) {
+continue;
+}
+
aio_context_acquire(ctx);
not_found = !bdrv_can_snapshot(bs);
aio_context_release(ctx);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 256609d..855a209 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -438,6 +438,9 @@ struct BlockDriverState {
/* do we need to tell the quest if we have a volatile write cache? */
int enable_write_cache;
+/* skip this BDS searching for one to save VM state */
+bool disable_vmstate_save;
+
/* the following member gives a name to every node on the bs graph. */
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
On 01/12/2016 08:40 PM, Markus Armbruster wrote:
Kevin Wolf writes:
Am 12.01.2016 um 17:35 hat Denis V. Lunev geschrieben:
On 01/12/2016 06:47 PM, Denis V. Lunev wrote:
On 01/12/2016 06:20 PM, Kevin Wolf wrote:
Am 12.01.2016 um 15:59 hat Paolo Bonzini geschrieben:
On 12/01/2016 15:16, Kevin Wolf wrote:
Thus we should avoid selection of "pflash" drives for VM
state saving.
For now "pflash" is read-write raw image as it configured by libvirt.
Thus there are no such images in the field and we could
safely disable
ability to save state to those images inside QEMU.
This is obviously broken. If you write to the pflash, then it needs to
be snapshotted in order to keep a consistent state.
If you want to avoid snapshotting the image, make it read-only and it
will be skipped even today.
Sort of. The point of having flash is to _not_ make it read-only, so
that is not a solution.
Flash is already being snapshotted as part of saving RAM state. In
fact, for this reason the device (at least the one used with OVMF; I
haven't checked other pflash devices) can simply save it back to disk
on the migration destination, without the need to use "migrate -b" or
shared storage.
[...]
I don't like very much using IF_PFLASH this way, which is why I hadn't
replied to the patch so far---I hadn't made up my mind about *what* to
suggest instead, or whether to just accept it. However, it does work.
Perhaps a separate "I know what I am doing" skip-snapshot option? Or
a device callback saying "not snapshotting this is fine"?
Boy, is this ugly...
What do you do with disk-only snapshots? The recovery only works as long
as you have VM state.
Kevin
actually I am in a bit of trouble :(
I understand that this is ugly, but I would like to make working
'virsh snapshot' for OVFM VMs. This is necessary for us to make
a release.
Currently libvirt guys generate XML in the following way:
hvm
/usr/share/OVMF/OVMF_CODE_new.fd
/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd
This results in:
qemu -drive
file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
\
-drive
file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd,if=pflash,format=raw,unit=1
This obviously can not pass check in bdrv_all_can_snapshot()
as 'pflash' is RW and raw, i.e. can not be snapshoted.
They have discussed the switch to the following command line:
qemu -drive
file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
\
-drive
file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd.qcow2,if=pflash,format=qcow2,unit=1
and say that in this case VM state could fall into PFLASH
drive which is should not be big as the location of the
file is different. This means that I am doomed here.
Either we should force libvirt people to forget about their
opinion that pflash should be small which I am unable to
do or I should invent a way to ban VM state saving into
pflash.
OK. There are 2 options.
1) Ban pflash as it was done.
2) Add 'no-vmstate' flag to -drive (invented just now).
something like this:
diff --git a/block.c b/block.c
index 3e1877d..8900589 100644
--- a/block.c
+++ b/block.c
@@ -881,6 +881,11 @@ static QemuOptsList bdrv_runtime_opts = {
.help = "Block driver to use for the node",
},
{
+.name = "novmstate",
+.type = QEMU_OPT_BOOL,
+.help = "Ignore for selecting to save VM state",
+},
+{
.name = BDRV_OPT_CACHE_WB,
.type = QEMU_OPT_BOOL,
.help = "Enable writeback mode",
@@ -957,6 +962,7 @@ static int bdrv_open_common(BlockDriverState
*bs, BdrvChild *file,
bs->request_alignment = 512;
bs->zero_beyond_eof = true;
bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
+bs->disable_vmstate_save = qemu_opt_get_bool(opts, "novmstate", false);
if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
error_setg(errp,
diff --git a/block/snapshot.c b/block/snapshot.c
index 2d86b88..33cdd86 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -483,6 +483,10 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
while (not_found && (bs = bdrv_next(bs))) {
AioContext *ctx = bdrv_get_aio_context(bs);
+if (bs->disable_vmstate_save) {
+continue;
+}
+
aio_context_acquire(ctx);
not_found = !bdrv_can_snapshot(bs);
aio_context_release(ctx);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 256609d..855a209 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -438,6 +438,9 @@ struct BlockDriverState {
/* do we need to tell the quest if we have a volatile write cache? */
int enable_write_cache;
+/* skip this BDS searching for one to save VM state */
+bool disable_vmstate_save;
+
/* the following member gives a name to every node on the bs graph. */
char node_name[32];
/* element of the list of
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
Am 12.01.2016 um 18:40 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 12.01.2016 um 17:35 hat Denis V. Lunev geschrieben:
> >> On 01/12/2016 06:47 PM, Denis V. Lunev wrote:
> >> >On 01/12/2016 06:20 PM, Kevin Wolf wrote:
> >> >>Am 12.01.2016 um 15:59 hat Paolo Bonzini geschrieben:
> >> >>>
> >> >>>On 12/01/2016 15:16, Kevin Wolf wrote:
> >> >Thus we should avoid selection of "pflash" drives for VM
> >> >state saving.
> >> >
> >> >For now "pflash" is read-write raw image as it configured by libvirt.
> >> >Thus there are no such images in the field and we could
> >> >safely disable
> >> >ability to save state to those images inside QEMU.
> >> This is obviously broken. If you write to the pflash, then it needs to
> >> be snapshotted in order to keep a consistent state.
> >>
> >> If you want to avoid snapshotting the image, make it read-only and it
> >> will be skipped even today.
> >> >>>Sort of. The point of having flash is to _not_ make it read-only, so
> >> >>>that is not a solution.
> >> >>>
> >> >>>Flash is already being snapshotted as part of saving RAM state. In
> >> >>>fact, for this reason the device (at least the one used with OVMF; I
> >> >>>haven't checked other pflash devices) can simply save it back to disk
> >> >>>on the migration destination, without the need to use "migrate -b" or
> >> >>>shared storage.
> >> >>>[...]
> >> >>>I don't like very much using IF_PFLASH this way, which is why I hadn't
> >> >>>replied to the patch so far---I hadn't made up my mind about *what* to
> >> >>>suggest instead, or whether to just accept it. However, it does work.
> >> >>>
> >> >>>Perhaps a separate "I know what I am doing" skip-snapshot option? Or
> >> >>>a device callback saying "not snapshotting this is fine"?
> >> >>Boy, is this ugly...
> >> >>
> >> >>What do you do with disk-only snapshots? The recovery only works as long
> >> >>as you have VM state.
> >> >>
> >> >>Kevin
> >> >actually I am in a bit of trouble :(
> >> >
> >> >I understand that this is ugly, but I would like to make working
> >> >'virsh snapshot' for OVFM VMs. This is necessary for us to make
> >> >a release.
> >> >
> >> >Currently libvirt guys generate XML in the following way:
> >> >
> >> >
> >> >hvm
> >> > >> >type='pflash'>/usr/share/OVMF/OVMF_CODE_new.fd
> >> >/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd
> >> >
> >> >
> >> >This results in:
> >> >
> >> >qemu -drive
> >> >file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
> >> >\
> >> > -drive
> >> > file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd,if=pflash,format=raw,unit=1
> >> >
> >> >This obviously can not pass check in bdrv_all_can_snapshot()
> >> >as 'pflash' is RW and raw, i.e. can not be snapshoted.
> >> >
> >> >They have discussed the switch to the following command line:
> >> >
> >> >qemu -drive
> >> >file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
> >> >\
> >> > -drive
> >> > file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd.qcow2,if=pflash,format=qcow2,unit=1
> >> >
> >> >and say that in this case VM state could fall into PFLASH
> >> >drive which is should not be big as the location of the
> >> >file is different. This means that I am doomed here.
> >> >
> >> >Either we should force libvirt people to forget about their
> >> >opinion that pflash should be small which I am unable to
> >> >do or I should invent a way to ban VM state saving into
> >> >pflash.
> >> >
> >> >OK. There are 2 options.
> >> >
> >> >1) Ban pflash as it was done.
> >> >2) Add 'no-vmstate' flag to -drive (invented just now).
> >> >
> >> something like this:
> >>
> >> diff --git a/block.c b/block.c
> >> index 3e1877d..8900589 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -881,6 +881,11 @@ static QemuOptsList bdrv_runtime_opts = {
> >> .help = "Block driver to use for the node",
> >> },
> >> {
> >> +.name = "novmstate",
> >> +.type = QEMU_OPT_BOOL,
> >> +.help = "Ignore for selecting to save VM state",
> >> +},
> >> +{
> >> .name = BDRV_OPT_CACHE_WB,
> >> .type = QEMU_OPT_BOOL,
> >> .help = "Enable writeback mode",
> >> @@ -957,6 +962,7 @@ static int bdrv_open_common(BlockDriverState
> >> *bs, BdrvChild *file,
> >> bs->request_alignment = 512;
> >> bs->zero_beyond_eof = true;
> >> bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
> >> +bs->disable_vmstate_save = qemu_opt_get_bool(opts, "novmstate",
> >> false);
> >>
> >> if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
> >> error_setg(errp,
> >> diff --git a/block/snapshot.c b/block/snapshot.c
> >> index 2d86b88..33cdd86 100644
> >> --- a/block/snapshot.c
> >> +++ b/block/snapshot.c
> >> @@ -483,6 +483,10 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
> >> while (not_found && (bs = bdrv_next(bs))) {
> >>
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
Kevin Wolf writes:
> Am 12.01.2016 um 17:35 hat Denis V. Lunev geschrieben:
>> On 01/12/2016 06:47 PM, Denis V. Lunev wrote:
>> >On 01/12/2016 06:20 PM, Kevin Wolf wrote:
>> >>Am 12.01.2016 um 15:59 hat Paolo Bonzini geschrieben:
>> >>>
>> >>>On 12/01/2016 15:16, Kevin Wolf wrote:
>> >Thus we should avoid selection of "pflash" drives for VM
>> >state saving.
>> >
>> >For now "pflash" is read-write raw image as it configured by libvirt.
>> >Thus there are no such images in the field and we could
>> >safely disable
>> >ability to save state to those images inside QEMU.
>> This is obviously broken. If you write to the pflash, then it needs to
>> be snapshotted in order to keep a consistent state.
>>
>> If you want to avoid snapshotting the image, make it read-only and it
>> will be skipped even today.
>> >>>Sort of. The point of having flash is to _not_ make it read-only, so
>> >>>that is not a solution.
>> >>>
>> >>>Flash is already being snapshotted as part of saving RAM state. In
>> >>>fact, for this reason the device (at least the one used with OVMF; I
>> >>>haven't checked other pflash devices) can simply save it back to disk
>> >>>on the migration destination, without the need to use "migrate -b" or
>> >>>shared storage.
>> >>>[...]
>> >>>I don't like very much using IF_PFLASH this way, which is why I hadn't
>> >>>replied to the patch so far---I hadn't made up my mind about *what* to
>> >>>suggest instead, or whether to just accept it. However, it does work.
>> >>>
>> >>>Perhaps a separate "I know what I am doing" skip-snapshot option? Or
>> >>>a device callback saying "not snapshotting this is fine"?
>> >>Boy, is this ugly...
>> >>
>> >>What do you do with disk-only snapshots? The recovery only works as long
>> >>as you have VM state.
>> >>
>> >>Kevin
>> >actually I am in a bit of trouble :(
>> >
>> >I understand that this is ugly, but I would like to make working
>> >'virsh snapshot' for OVFM VMs. This is necessary for us to make
>> >a release.
>> >
>> >Currently libvirt guys generate XML in the following way:
>> >
>> >
>> >hvm
>> >> >type='pflash'>/usr/share/OVMF/OVMF_CODE_new.fd
>> >/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd
>> >
>> >
>> >This results in:
>> >
>> >qemu -drive
>> >file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
>> >\
>> > -drive
>> > file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd,if=pflash,format=raw,unit=1
>> >
>> >This obviously can not pass check in bdrv_all_can_snapshot()
>> >as 'pflash' is RW and raw, i.e. can not be snapshoted.
>> >
>> >They have discussed the switch to the following command line:
>> >
>> >qemu -drive
>> >file=/usr/share/OVMF/OVMF_CODE_new.fd,if=pflash,format=raw,unit=0,readonly=on
>> >\
>> > -drive
>> > file=/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd.qcow2,if=pflash,format=qcow2,unit=1
>> >
>> >and say that in this case VM state could fall into PFLASH
>> >drive which is should not be big as the location of the
>> >file is different. This means that I am doomed here.
>> >
>> >Either we should force libvirt people to forget about their
>> >opinion that pflash should be small which I am unable to
>> >do or I should invent a way to ban VM state saving into
>> >pflash.
>> >
>> >OK. There are 2 options.
>> >
>> >1) Ban pflash as it was done.
>> >2) Add 'no-vmstate' flag to -drive (invented just now).
>> >
>> something like this:
>>
>> diff --git a/block.c b/block.c
>> index 3e1877d..8900589 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -881,6 +881,11 @@ static QemuOptsList bdrv_runtime_opts = {
>> .help = "Block driver to use for the node",
>> },
>> {
>> +.name = "novmstate",
>> +.type = QEMU_OPT_BOOL,
>> +.help = "Ignore for selecting to save VM state",
>> +},
>> +{
>> .name = BDRV_OPT_CACHE_WB,
>> .type = QEMU_OPT_BOOL,
>> .help = "Enable writeback mode",
>> @@ -957,6 +962,7 @@ static int bdrv_open_common(BlockDriverState
>> *bs, BdrvChild *file,
>> bs->request_alignment = 512;
>> bs->zero_beyond_eof = true;
>> bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
>> +bs->disable_vmstate_save = qemu_opt_get_bool(opts, "novmstate", false);
>>
>> if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
>> error_setg(errp,
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 2d86b88..33cdd86 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -483,6 +483,10 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
>> while (not_found && (bs = bdrv_next(bs))) {
>> AioContext *ctx = bdrv_get_aio_context(bs);
>>
>> +if (bs->disable_vmstate_save) {
>> +continue;
>> +}
>> +
>> aio_context_acquire(ctx);
>> not_found = !bdrv_can_snapshot(bs);
>> aio_context_release(ctx);
>> diff --git a/include/block/block_int.h b/inclu
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
Am 12.01.2016 um 16:16 hat Peter Maydell geschrieben: > On 12 January 2016 at 15:13, Denis V. Lunev wrote: > > The idea of this patch was trivial. First of all, I would like to keep > > this image internally snapshoted. That is why the ultimate goal > > was to switch from raw to qcow2 to keep changes inside the > > image. > > > > Though in this case this drive could be selected to save VM > > state, which could be big. The function being changed selects > > the image for VM state saving. > > > > here I would like to skip IP_PFLASH from being selected to keep > > it small as required by libvirt guys. > > This has to be a board specific decision. Some of our machine > models might have no backing storage other than an IP_PFLASH > drive, but it's still nice to be able to do vmsave/vmload on them. Maybe we can give flash images lower priority than other images? I'm not sure if we don't break compatibility with such a change, though. loadvm on existing snapshots could fail now. We might need to change that first so that it can find snapshots even on images that wouldn't be the VM state image for new snapshots any more. Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
On 12 January 2016 at 15:13, Denis V. Lunev wrote: > The idea of this patch was trivial. First of all, I would like to keep > this image internally snapshoted. That is why the ultimate goal > was to switch from raw to qcow2 to keep changes inside the > image. > > Though in this case this drive could be selected to save VM > state, which could be big. The function being changed selects > the image for VM state saving. > > here I would like to skip IP_PFLASH from being selected to keep > it small as required by libvirt guys. This has to be a board specific decision. Some of our machine models might have no backing storage other than an IP_PFLASH drive, but it's still nice to be able to do vmsave/vmload on them. thanks -- PMM
