Re: [Qemu-devel] [PATCH 1/2] commit: Add top-node/base-node options

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4319 bytes --]

Am 04.09.2018 um 17:00 hat Peter Krempa geschrieben:
> On Tue, Sep 04, 2018 at 16:42:17 +0200, Alberto Garcia wrote:
> > On Tue 04 Sep 2018 04:17:30 PM CEST, Peter Krempa wrote:
> > >> >> libvirt-3-format is read-write and all other node names are
> > >> >> readonly in the above example.
> > >> >> 
> > >> >> The same also happens when using filenames:
> > >> >> 
> > >> >> {"execute":"block-commit",
> > >> >>  "arguments" : {"device":"libvirt-3-format",
> > >> >>                 "job-id":"libvirt-3-format",
> > >> >>                 "top":"/var/lib/libvirt/images/rhel7.3.1483615252",
> > >> >>                 "base":"/var/lib/libvirt/images/rhel7.3.1483605924",
> > >> >>                 "auto-finalize":true,
> > >> >>                 "auto-dismiss":false},
> > >> >>  "id":"libvirt-13"}
> > >> >> 
> > >> >> {"id":"libvirt-13","error":{"class":"GenericError","desc":"Block node is read-only"}}
> > >> >
> > >> > I see what's happening here. So we have a graph like this:
> > >> >
> > >> >      guest device
> > >> >           |
> > >> >           v
> > >> >     overlay-format -------> backing-format
> > >> >     [read-only=off]         [read-only=on]
> > >> >           |                        |
> > >> >           v                        v
> > >> >     overlay-proto           backing-proto
> > >> >     [read-only=off]         [read-only=on]
> > >> >
> > >> > The difference between your -blockdev use and -drive is that you
> > >> > explicitly specify the read-only option for backing-proto (and you use
> > >> > a separate -blockdev option anyway), so it doesn't just inherit it
> > >> > from backing-format.
> > >> 
> > >> Are these format and protocol block devices opened with four separate
> > >> -blockdev parameters? Is that how libvirt does it?
> > >
> > > Yes. This goes along with the fact that for 'blockdev-create' you need
> > > to blockdev-add the file which you want to format, but the formatted
> > > file is not automatically added.
> > >
> > > If we'd use the approach where the protocol layer is opened as part of
> > > the format layer it would complicate the snapshot code where we need
> > > to add a file and then format it to qcow2. It would mean that we'd
> > > have to blockdev-add a file, format it via blockdev-create, then
> > > blockdev-del it and open it together with the format layer. Otherwise
> > > the disk hot-unplug code would be plain crazy.
> > 
> > Do you need to add the protocol layer in order to format it, though? :-?
> > 
> > (I'm just trying to understand how this works, I'm not too familiar with
> > blockdev-create)
> > 
> > {'execute': 'blockdev-create',
> >  'arguments': {'job-id': 'job0',
> >                'options': {'driver': 'file',
> >                            'filename': 'test.qcow2',
> >                            'size': 0}}}
> > {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
> > 
> > {'execute': 'blockdev-create',
> >  'arguments': {'job-id': 'job1',
> >                'options': { 'driver': 'qcow2',
> >                             'size': 1048576,
> >                             'file': {'driver': 'file',
> >                                      'filename': 'test.qcow2'}}}}
> > {'execute': 'job-dismiss', 'arguments': {'id': 'job1'}}
> 
> Honestly I've reused the existing approach and did not try without
> actually adding the protocol layer.
> 
> I remember being told some time ago to specify both layers explicitly.
> Since it's not yet enabled in libvirt we theoretically could change to
> one -blockdev for format+protocol but in that case we need some kind
> of guarantee that every (even new) feature will work with it.
> 
> Switching between those approaches once we enable it upstream will not
> be possible without adding a lot of compatibility code.

Yeah, I think specifying both layers explicitly is cleaner. This should
probably be solved some way inside QEMU.

The read-only option for the backend isn't that useful anyway. Maybe we
should do away with it, at least for its current purpose, and just rely
on write permissions taken by parents. We could then either silently
ignore (and deprecate) the read-only backend option or we could change
its semantics to mean "never allow a writer on this node".

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]