Re: [Qemu-devel] [PATCH 5/6] add-cow: support snapshot_blkdev

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 19 Jul 2012 10:17:10 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 19.07.2012 04:20, schrieb Dong Xu Wang:
> > On Thu, Jun 14, 2012 at 7:33 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> >> Am 14.06.2012 13:18, schrieb Paolo Bonzini:
> >>> Il 14/06/2012 12:59, Kevin Wolf ha scritto:
> >>>> Am 13.06.2012 16:36, schrieb Dong Xu Wang:
> >>>>> add-cow will let raw file support snapshot_blkdev indirectly.
> >>>>>
> >>>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> >>>>
> >>>> Paolo, what do you think about this magic?
> >>>
> >>> Besides the obvious layering violation (it would be better to add a new
> >>> method bdrv_ext_snapshot_create perhaps) I don't see very much a problem
> >>> in it.  Passing image creation options sounds like a good idea that we
> >>> can add in the future as an extension.
> >>>
> >>> But honestly, I don't really see the point of add-cow in general...  The
> >>> raw image is anyway not usable without a pass through qemu-io convert,
> >>> and mirroring will also allow collapsing an image to raw (with the
> >>> persistent dirty bitmap playing the role of the add-cow metadata).
> >>
> >> Well, the idea was that you stream into add-cow and once the streaming
> >> has completed, you can just drop the bitmap.
> >>
> >> There might be some overlap with mirroring, though when we discussed
> >> introducing add-cow, mirrors were not yet on the table. One difference
> >> that remains is that with streaming the guest only writes to the target
> >> image and can't have any problem with convergence.
> >>
> >>>> I think I can see its use especially for HMP because it's quite
> >>>> convenient, but on the other hand it assumes a fixed image path for the
> >>>> new raw image. This isn't going to work for block devices, for example.
> >>>
> >>> True, but then probably you would use mode='existing', because you need
> >>> to pre-create the logical volume.
> >>
> >> I think it might be convenient to have the raw volume precreated (you
> >> need to do that anyway), but create the COW bitmap only during the
> >> snapshot command. But yeah, not really important.
> >>
> >>>> If we don't do it this way, we need to allow passing image creation
> >>>> options to the snapshotting command so that you can pass a precreated
> >>>> image file.
> >>>
> >>> This sounds like a useful extension anyway, except that passing an
> >>> unstructured string for image creation options is ugly...  Perhaps we
> >>> can base a better implementation of options on Laszlo's QemuOpts visitor.
> >>
> >> Yes, I wouldn't want to use a string here, we should use something
> >> structured. Image creation still uses the old-style options instead of
> >> QemuOpts, but maybe this is the opportunity to convert it.
> > 
> > Kevin, do you mean I need to replace QEMUOptionParameter with QemuOpts?
> > 
> > If true, other image formats should also be changed, I noticed Stefan
> > has an un-comleted patch:
> > http://repo.or.cz/w/qemu/stefanha.git/commitdiff/b49babb2c8b476a36357cfd7276ca45a11039ca5
> > 
> > then I can work based on Stefan's patch.
> 
> Yes, the direction of this patch from Stefan's repo looks good.
> 
> I'm not sure whether it will immediately allow passing structured image
> creation options in QMP, though. It might work using the old-style
> monitor commands, just getting a QDict from the options. Not quite sure
> how this is going to work with QAPI.

We did it for netdev_add and it works as you said above. The schema
entry looks like this:

 { 'command': 'netdev_add',
   'data': {'type': 'str', 'id': 'str', '*props': '**'},
   'gen': 'no' }

Where 'props' is:

 # @props: #optional a list of properties to be passed to the backend in
 #         the format 'name=value', like 'ifname=tap0,script=no'


Then we have the qmp front-end, using the old style monitor signature:

 int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret);

And we have the HMP front-end (which should be used elsewhere too):

 void netdev_add(QemuOpts *opts, Error **errp);

As Paolo said, Laszlo's work will help us to this the right way.