Re: [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot

["Denis V. Lunev" <den@openvz.org>]

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:
>
>   <os>
>     <type arch='x86_64' machine='pc-i440fx-2.3'>hvm</type>
>     <loader readonly='yes' 
> type='pflash'>/usr/share/OVMF/OVMF_CODE_new.fd</loader>
> <nvram>/var/lib/libvirt/qemu/nvram/f20efi_VARS.fd</nvram>
>   </os>
>
> 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 named nodes building the graph */