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