[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing fil
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen |
Date: |
Wed, 20 Jun 2018 12:58:55 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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
- [Qemu-devel] [RFC PATCH 03/10] block: Allow changing 'detect-zeroes' on reopen, (continued)
- [Qemu-devel] [RFC PATCH 03/10] block: Allow changing 'detect-zeroes' on reopen, Alberto Garcia, 2018/06/14
- [Qemu-devel] [RFC PATCH 02/10] block: Allow changing 'discard' on reopen, Alberto Garcia, 2018/06/14
- [Qemu-devel] [RFC PATCH 01/10] file-posix: Forbid trying to change unsupported options during reopen, Alberto Garcia, 2018/06/14
- [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen, Alberto Garcia, 2018/06/14
- Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen, Kevin Wolf, 2018/06/18
- Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen, Alberto Garcia, 2018/06/18
- Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen, Kevin Wolf, 2018/06/18
- Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen, Alberto Garcia, 2018/06/19
- Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen, Kevin Wolf, 2018/06/19
- Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen, Alberto Garcia, 2018/06/19
- Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen,
Kevin Wolf <=
- Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen, Alberto Garcia, 2018/06/20
- Re: [Qemu-devel] [RFC PATCH 06/10] block: Allow changing the backing file on reopen, Kevin Wolf, 2018/06/21
[Qemu-devel] [RFC PATCH 09/10] block: Add a 'x-blockdev-reopen' QMP command, Alberto Garcia, 2018/06/14
[Qemu-devel] [RFC PATCH 08/10] block: Add bdrv_reset_options_allowed(), Alberto Garcia, 2018/06/14
[Qemu-devel] [RFC PATCH 05/10] block: Add 'keep_old_opts' parameter to bdrv_reopen_queue(), Alberto Garcia, 2018/06/14
[Qemu-devel] [RFC PATCH 07/10] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver, Alberto Garcia, 2018/06/14
[Qemu-devel] [RFC PATCH 10/10] qemu-iotests: Test the x-blockdev-reopen QMP command, Alberto Garcia, 2018/06/14