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

[Peter Krempa]

On Mon, Sep 03, 2018 at 17:03:11 +0200, Kevin Wolf wrote:
> Am 28.08.2018 um 16:26 hat Peter Krempa geschrieben:
> > On Fri, Aug 10, 2018 at 18:26:57 +0200, Kevin Wolf wrote:
> > > The block-commit QMP command required specifying the top and base nodes
> > > of the commit jobs using the file name of that node. While this works
> > > in simple cases (local files with absolute paths), the file names
> > > generated for more complicated setups can be hard to predict.
> > > 
> > > This adds two new options top-node and base-node to the command, which
> > > allow specifying node names instead. They are mutually exclusive with
> > > the old options.
> > > 
> > > Signed-off-by: Kevin Wolf <address@hidden>
> > > ---
> > >  qapi/block-core.json | 24 ++++++++++++++++++------
> > >  blockdev.c           | 32 ++++++++++++++++++++++++++++++--
> > >  2 files changed, 48 insertions(+), 8 deletions(-)
> > 
> > While the below is not strictly relevant to this patch usage
> > block-commit is not possible when using -blockdev. Thus the new
> > arguments are not very useful otherwise.
> > 
> > With the new options I'm getting:
> > 
> > {"execute":"block-commit",
> >  "arguments": { "device":"libvirt-3-format",
> >                 "job-id":"libvirt-3-format",
> >                 "top-node":"libvirt-8-format",
> >                 "base-node":"libvirt-9-format",
> >                 "auto-finalize":true,
> >                 "auto-dismiss":false},
> >  "id":"libvirt-16"}
> > 
> > {"id":"libvirt-16",
> >  "error":{"class":"GenericError",
> >           "desc":"Block node is read-only"}}
> > 
> > I'm pointing into the backing chain so the files are declared as read-only.
> > 
> > It works just-fine if I open them as read-write with
> > -blockdev/blockdev-add but that obviously is not correct as you can't
> > then share parts of the backing chain with other VMs due to image
> > locking.
> > 
> > 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.
> 
> Now, when the commit job tries to reopen backing-format, your explicit
> read-only=on for backing-proto still takes precedence and the node stays
> read-only. If you hadn't used a separate -blockdev for backing-proto,
> but included it in the definition for backing-format and left out the
> read-only option, it would have inherited the option and reopen would
> adjust both nodes. This is what happens with -drive.
> 
> So essentially, I guess, all places that want to switch between
> read-only and read-write need to learn which other nodes (apart from the
> top-level node they are interested in) must be reopened as well.

We theoretically can always open the protocol layer read-write if it
does not conflict with the image locking code (I did not test that).

Changing to opening them as dependancy of the format layer would
complicate things with blockdev-create where that would not be possible
and would require blockdev-add(proto), blockdev-create,
blockdev-del(proto),blockdev-add (format+proto).

signature.asc
Description: PGP signature