From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41486) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fVcKe-0001fh-JE for qemu-devel@nongnu.org; Wed, 20 Jun 2018 08:35:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fVcKb-0001Hu-C0 for qemu-devel@nongnu.org; Wed, 20 Jun 2018 08:35:28 -0400 From: Alberto Garcia In-Reply-To: <20180620105854.GC3946@localhost.localdomain> References: <6782db91216488e4fc1509a267a362a3f15a1aaa.1528991017.git.berto@igalia.com> <20180618143801.GC4667@localhost.localdomain> <20180618161229.GD4667@localhost.localdomain> <20180619130509.GA21115@dhcp-200-186.str.redhat.com> <20180620105854.GC3946@localhost.localdomain> Date: Wed, 20 Jun 2018 14:35:23 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz , Eric Blake , Markus Armbruster 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