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