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

[Denis V. Lunev]

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 */