Re: Plans to bring QMP 'x-blockdev-reopen' out of experimental?

[Kevin Wolf]

Am 02.12.2020 um 17:12 hat Alberto Garcia geschrieben:
> On Tue 20 Oct 2020 10:23:33 AM CEST, Kevin Wolf wrote:
> >> I forgot to add, we still don't support changing bs->file with this
> >> command, so I guess that would be one blocker?
> >> 
> >> There's no other way of inserting filter nodes, or is there?
> >
> > Not that I'm aware of.
> >
> > So yes, changing bs->file is the one thing I had in mind for
> > implementing before we mark it stable.
> >
> > I'm not entirely sure if we should make some restrictions or allow
> > arbitrary changes. If it's only about filters, we could check that the
> > node returned by bdrv_skip_filters() stays the same. This would be
> > almost certainly safe (if the chain is not frozen, of course).
> >
> > If people want to dynamically insert non-filters (maybe quorum?), it
> > might be more restrictive than necessary, though.
> >
> > Other cases like inserting a qcow2 file in the chain where the old
> > child becomes the backing file of the newly inserted node should in
> > theory already be covered by blockdev-snapshot.
> 
> Hi,
> 
> I have been working a bit on this

Oh, nice! And you might have mentioned this just in time to stop me from
duplicating your work. There is a strong desire from libvirt to have a
stable blockdev-reopen in QEMU 6.0.

> and I have doubts about the following situation: let's say we have a
> normal qcow2 image with two BDS for format (node-name "hd0") and
> protocol ("hd0-file"):
> 
>    hd0 -> hd0-file
> 
> { "execute": "blockdev-add", "arguments":
>    {'driver': 'file', 'node-name': 'hd0-file', 'filename':  'hd0.qcow2 }}
> { "execute": "blockdev-add", "arguments":
>    {'driver': 'qcow2', 'node-name': 'hd0', 'file': 'hd0-file'}}
> 
> Now we want to use x-blockdev-reopen to insert a throttle filter
> between them, so the result would be:
> 
>    hd0 -> throttle -> hd0-file
> 
> First we add the filter:
> 
> { "execute": "object-add", "arguments":
>    { 'qom-type': 'throttle-group', 'id': 'group0',
>      'props': { 'limits': { 'iops-total': 1000 } } } }
> { "execute": "blockdev-add", "arguments":
>    { 'driver': 'throttle', 'node-name': 'throttle0',
>      'throttle-group': 'group0', 'file': "hd0-file" } }
> 
> And then we insert it:
> 
> { "execute": "x-blockdev-reopen", "arguments":
>    {'driver': 'qcow2', 'node-name': 'hd0', 'file': 'throttle0'}}
> 
> So x-blockdev-reopen sees that we want to replace the current bs->file
> ("hd0-file") with a new one ("throttle0"). The problem here is that
> throttle0 has hd0-file as its child, so when we check the permissions on
> throttle0 (and its children) we get that hd0-file refuses because it's
> already being used (although in in the process of being replaced) by
> hd0:
> 
> "Conflicts with use by hd0 as 'file', which does not allow 'write, resize' on 
> hd0-file"
> 
> And we would get a similar problem with the reverse operation (removing
> the filter).

This kind of situation isn't new, I believe some of the existing graph
changes (iirc in the context of block jobs) can cause the same problem.

This is essentially why some functions in the permission system take a
GSList *ignore_children. So I believe the right thing to do here is
telling the permission system that it needs to check the situation
without the BdrvChild that links hd0 with hd0-file.

I don't know the exact stack trace of your failure, so maybe this
parameter isn't available yet in the place where you need it, but in the
core functions it exists.

Does this help or am I missing some details?

Kevin