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

[Kevin Wolf <kwolf@redhat.com>]

Am 20.06.2018 um 14:35 hat Alberto Garcia geschrieben:
> 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.

Uh, and that works?

Clearing BDRV_O_INACTIVE with bdrv_reopen() (which means, without
calling bdrv_invalidate_cache()) is surely suspicious. Probably this
code is buggy.

Another reason for removing flags from the interface: We didn't do any
sanity checks for the flag changes.

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

It provides -r/-w for changing readonly and -c to change the cache mode
flags. Both should be easy to convert to QDict options.

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

Not sure, maybe we don't even need them now that we decided that the
default is leaving the reference unchanged.

There's the creation of json: filenames, maybe we need it there. We'd
also certainly need to get the full QDict if we wanted to convert the
options to a QAPI object before we pass them to the drivers.

Kevin