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

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

[Alberto Garcia]

On Tue 06 Oct 2020 11:10:01 AM CEST, Kashyap Chamarthy wrote:
> Hi, folks
>
> If this was already discussed on the list, please point me to the
> thread.  I took a quick look at my local archives, I didn't find any,
> besides patches to tests.

I think this is the last time that I was discussed:

   https://lists.gnu.org/archive/html/qemu-block/2020-02/msg00558.html

And this one in particular:

   https://lists.gnu.org/archive/html/qemu-block/2020-02/msg00601.html

But Kevin fixed this one month later:

   https://lists.gnu.org/archive/html/qemu-block/2020-03/msg00266.html

Berto

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

[Alberto Garcia]

On Mon 19 Oct 2020 05:56:56 PM CEST, Alberto Garcia wrote:
> And this one in particular:
>
>    https://lists.gnu.org/archive/html/qemu-block/2020-02/msg00601.html

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?

Berto

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

[Kevin Wolf]

Am 19.10.2020 um 18:46 hat Alberto Garcia geschrieben:
> On Mon 19 Oct 2020 05:56:56 PM CEST, Alberto Garcia wrote:
> > And this one in particular:
> >
> >    https://lists.gnu.org/archive/html/qemu-block/2020-02/msg00601.html
> 
> 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.

Kevin

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

[Alberto Garcia]

On Tue 20 Oct 2020 10:23:33 AM CEST, Kevin Wolf wrote:
>> >    https://lists.gnu.org/archive/html/qemu-block/2020-02/msg00601.html
>> 
>> 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.

Note that you can still open a new bs with a different bs->file and
replace the original one (as long as the original one is the backing
file of an existing bs, that is :)).

> 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.

You mean replacing bs->file with a Quorum node? Quorum itself does not
use bs->file, it has the 'children' attribute.

Berto

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

[Kevin Wolf]

Am 20.10.2020 um 13:53 hat Alberto Garcia geschrieben:
> On Tue 20 Oct 2020 10:23:33 AM CEST, Kevin Wolf wrote:
> >> >    https://lists.gnu.org/archive/html/qemu-block/2020-02/msg00601.html
> >> 
> >> 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.
> 
> Note that you can still open a new bs with a different bs->file and
> replace the original one (as long as the original one is the backing
> file of an existing bs, that is :)).

You can't open the same image twice, so this won't always work.

> > 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.
> 
> You mean replacing bs->file with a Quorum node? Quorum itself does not
> use bs->file, it has the 'children' attribute.

Yes, replaying bs->file with a Quorum node that has the old bs->file as
one of its children. Or removing a Quorum node so that one of it's
children replaces it.

But this is probably not a very important use case, so I think the
restriction is not a problem. Lifting restrictions later is easier than
adding new ones.

Kevin

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

[Alberto Garcia]

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 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).

Berto

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

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

[Alberto Garcia]

On Wed 02 Dec 2020 05:28:08 PM CET, Kevin Wolf wrote:

>> 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"
>> 
> 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 had tried this already and it does work when inserting the filter (we
know that 'hd0-file' is about to be detached from the parent so we can
put it in the list) but I don't think it's so easy if we want to remove
the filter, i.e.

   hd0 -> throttle -> hd0-file     ======>     hd0 -> hd0-file

In this case we get a similar error, we want to make hd0-file a child of
hd0 but it is being used by the throttle filter.

Telling bdrv_check_update_perm() to ignore hd0's current child
(throttle) won't solve the problem.

> 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.

This is in bdrv_reopen_multiple(), in the same place where we are
currently checking the permissions of the new backing file.

Berto

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

[Kevin Wolf]

Am 02.12.2020 um 17:40 hat Alberto Garcia geschrieben:
> On Wed 02 Dec 2020 05:28:08 PM CET, Kevin Wolf wrote:
> 
> >> 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"
> >> 
> > 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 had tried this already and it does work when inserting the filter (we
> know that 'hd0-file' is about to be detached from the parent so we can
> put it in the list) but I don't think it's so easy if we want to remove
> the filter, i.e.
> 
>    hd0 -> throttle -> hd0-file     ======>     hd0 -> hd0-file
> 
> In this case we get a similar error, we want to make hd0-file a child of
> hd0 but it is being used by the throttle filter.
> 
> Telling bdrv_check_update_perm() to ignore hd0's current child
> (throttle) won't solve the problem.

Isn't this the very same case as removing e.g. a mirror filter from the
chain? I'm sure we have already solved this somewhere.

Hm, no, it might actually be different in that the throttle node
survives this, so we do have to check that the resulting graph is
valid. Do we need a combined operation to remove the throttle node from
the graph and immediately delete it?

> > 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.
> 
> This is in bdrv_reopen_multiple(), in the same place where we are
> currently checking the permissions of the new backing file.

Oh, it's not happening while actually changing the links, but the check
before trying? I guess both would fail in this case anyway, but good to
know.

Kevin

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

[Alberto Garcia]

On Wed 02 Dec 2020 06:51:21 PM CET, Kevin Wolf wrote:
>> I had tried this already and it does work when inserting the filter (we
>> know that 'hd0-file' is about to be detached from the parent so we can
>> put it in the list) but I don't think it's so easy if we want to remove
>> the filter, i.e.
>> 
>>    hd0 -> throttle -> hd0-file     ======>     hd0 -> hd0-file
>> 
>> In this case we get a similar error, we want to make hd0-file a child of
>> hd0 but it is being used by the throttle filter.
>> 
>> Telling bdrv_check_update_perm() to ignore hd0's current child
>> (throttle) won't solve the problem.
>
> Isn't this the very same case as removing e.g. a mirror filter from the
> chain? I'm sure we have already solved this somewhere.
>
> Hm, no, it might actually be different in that the throttle node
> survives this, so we do have to check that the resulting graph is
> valid. Do we need a combined operation to remove the throttle node
> from the graph and immediately delete it?

What kind of API are you thinking about?

One basic problem with inserting filter nodes (as opposed to replacing a
node with a different one) is that we have a protocol BDS used twice at
the same time, e.g.

  hd0 -> hd0-file
  throttle -> hd0-file

And then we would reopen hd0 and do the change, but ideally one would
prefer to avoid having hd0-file twice.

There's also the x-blockdev-change function (currently for Quorum only),
I wonder if it could be a better fit for this use case.

Berto

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

[Kashyap Chamarthy]

On Mon, Oct 19, 2020 at 05:56:56PM +0200, Alberto Garcia wrote:
> On Tue 06 Oct 2020 11:10:01 AM CEST, Kashyap Chamarthy wrote:
> > Hi, folks
> >
> > If this was already discussed on the list, please point me to the
> > thread.  I took a quick look at my local archives, I didn't find any,
> > besides patches to tests.
> 
> I think this is the last time that I was discussed:
> 
>    https://lists.gnu.org/archive/html/qemu-block/2020-02/msg00558.html
> 
> And this one in particular:
> 
>    https://lists.gnu.org/archive/html/qemu-block/2020-02/msg00601.html
> 
> But Kevin fixed this one month later:
> 
>    https://lists.gnu.org/archive/html/qemu-block/2020-03/msg00266.html

Ah, thanks for these URLs, Berto.  My bad, I didn't find it in my local
archives because of the way I organized my mail filters; I recently
cleared the 'qemu-block' maildir, so these didn't show up.

-- 
/kashyap