Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen

[Kevin Wolf]

Am 19.06.2018 um 16:20 hat Alberto Garcia geschrieben:
> >> Wait, I think the description I gave is inaccurate:
> >> 
> >> commit_complete() calls bdrv_drop_intermediate(), and that updates the
> >> backing image name (c->role->update_filename()). If we're doing this in
> >> an intermediate node then it needs to be reopened in read-write mode,
> >> while keeping the rest of the options.
> >> 
> >> But then bdrv_reopen_commit() realizes that the backing file (from
> >> reopen_state->options) is not the current one (because there's a
> >> bdrv_mirror_top implicit filter) and attempts to remove it. And that's
> >> the root cause of the crash.
> >
> > How did the other (the real?) backing file get into
> > reopen_state->options? I don't think bdrv_drop_intermediate() wants to
> > change anything except the read-only flag, so we should surely have
> > the node name of bdrv_mirror_top there?
> 
> No, it doesn't (try to) change anything else. It cannot do it:
> bdrv_reopen() only takes flags, but keeps the current options. And the
> current options have 'backing' set to a thing different from the
> bdrv_mirror_top node.

Okay, so in theory this is expected to just work.

Actually, do we ever use bdrv_reopen() for flags other than read-only?
Maybe we should get rid of that flags nonsense and simply make it a
bdrv_reopen_set_readonly() taking a boolean.

> > > So my first impression is that we should not try to change backing
> > > files if a reopen was not requested by the user (blockdev-reopen)
> > > and perhaps we should forbid it when there are implicit nodes
> > > involved.
> > Changing the behaviour depending on who the caller is sounds like a
> > hack that relies on coincidence and may break sooner or later.
> 
> The main difference between the user calling blockdev-reopen and the
> code doing bdrv_reopen() internally is that in the former case all
> existing options are ignored (keep_old_opts = false) and in the latter
> they are kept.
> 
> This latter case can have unintended consequences, and I think they're
> all related to the fact that bs->explicit_options also keeps the options
> of its children. So if you have
> 
>    C <- B <- A
> 
> and A contains 'backing.driver=qcow2, backing.node-name=foo, ...', and
> you reopen A (keeping its options) then everything goes fine. However if
> you remove B from the chain (using e.g. block-stream) then you can't
> reopen A anymore because backing.* no longer matches the current backing
> file.
> 
> I suppose that we would need to remove the children options from
> bs->explicit_options in that case? If this all happens because of the
> user calling blockdev-reopen then we don't need to worry about it becase
> bs->explicit_options are ignored.

So the problem is that bs->explicit_options (and probably bs->options)
aren't consistent with the actual state of the graph. The fix for that
is likely not in bdrv_reopen().

I think we should already remove nested options of children from the
dicts in bdrv_open_inherit(). I'm less sure about node-name references.
Maybe instead of keeing the dicts up-to-date each time we modify the
graph, we should just get rid of those in the dicts as well, and instead
add a function that reconstructs the full dict from bs->options and the
currently attached children. This requires that the child name and the
option name match, but I think that's true. (Mostly at least - what
about quorum? But the child node handling of quorum is broken anyway.)

I'm almost sure Max has an opinion about this, too. :-)

> >> Ah, ok, I think that's related to the crash that I mentioned earlier
> >> with block-commit. Yes, it makes sense that we forbid that. I suppose
> >> that we can do this already with BLK_PERM_GRAPH_MOD ?
> >
> > Possibly. The thing with BLK_PERM_GRAPH_MOD is that nobody knows what
> > its meaning has to be so that it's actually useful, so we're not using
> > it in any meaningful way yet.
> >
> > I suspect BLK_PERM_GRAPH_MOD is the wrong tool because what we
> > actually want to protect against modification is not a BDS, but a
> > BdrvChild. But I may be wrong there.
> 
> I'll take a closer look and see what I come up with.

Okay. Maybe don't implement anything yet, but just try to come up with a
concept for discussion.

> At the moment there's 
> 
> Berto

And it's very good to have a Berto at the moment, but I think you wanted
to add something else there? ;-)

Kevin