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

[Alberto Garcia]

On Wed 20 Jun 2018 12:58:55 PM CEST, Kevin Wolf wrote:
> 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.

I think that's a good idea. There's however one case where other flags
are changed: reopen_backing_file() in block/replication.c that also
clears BDRV_O_INACTIVE. I'd need to take a closer look to that code to
see what we can do about it.

And there's of course qemu-io's "reopen" command, which does take
options but keeps the previous values.

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

Probably not because the graph can be changed by other means (e.g
block-stream as I just said).

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

Yes, removing nested options sounds like a good idea. In what cases do
we need the full qdict, though?

>> 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? ;-)

I think it was just a leftover from a previous paragraph :-)

Berto