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

[Kevin Wolf]

Am 19.06.2018 um 14:27 hat Alberto Garcia geschrieben:
> On Mon 18 Jun 2018 06:12:29 PM CEST, Kevin Wolf wrote:
> >> >> This patch allows the user to change the backing file of an image
> >> >> that is being reopened. Here's what it does:
> >> >> 
> >> >>  - In bdrv_reopen_queue_child(): if the 'backing' option points to an
> >> >>    image different from the current backing file then it means that
> >> >>    the latter is going be detached so it must not be put in the reopen
> >> >>    queue.
> >> >> 
> >> >>  - In bdrv_reopen_prepare(): check that the value of 'backing' points
> >> >>    to an existing node or is null. If it points to an existing node it
> >> >>    also needs to make sure that replacing the backing file will not
> >> >>    create a cycle in the node graph (i.e. you cannot reach the parent
> >> >>    from the new backing file).
> >> >> 
> >> >>  - In bdrv_reopen_commit(): perform the actual node replacement by
> >> >>    calling bdrv_set_backing_hd(). It may happen that there are
> >> >>    temporary implicit nodes between the BDS that we are reopening and
> >> >>    the backing file that we want to replace (e.g. a commit fiter node),
> >> >>    so we must skip them.
> >> >
> >> > I think we shouldn't do this. blockdev-reopen is an advanced command
> >> > that requires knowledge of the graph at the node level. Therefore we can
> >> > expect the management tool to consider filter nodes when it reconfigures
> >> > the graph. This is important because it makes a difference whether a
> >> > new node is inserted above or below the filter node.
> >> 
> >> But those nodes that the management tool knows about would not be
> >> implicit, or would they?
> >
> > Hm, you're right, they are only implicit if no node name was given. So
> > it's not as bad as I thought.
> >
> > I still think of bs->implicit as a hack to maintain compatibility for
> > legacy commands rather than something to use in new commands.
> 
> Yes, and I'm still not 100% convinced that skipping the implicit nodes
> as I'm doing here is the proper solution, so maybe I'll need to come up
> with something else.
> 
> >> The reason why I did this is because there's several places in the
> >> QEMU codebase where bdrv_reopen() is called while there's a temporary
> >> implicit node. For example, on exit bdrv_commit() needs to call
> >> bdrv_set_backing_hd() to remove intermediate nodes from the
> >> chain. This happens while bdrv_mirror_top is still there, so if we
> >> don't skip it then QEMU crashes.
> >
> > But isn't that bdrv_set_backing_hd() a separate operation? I would
> > understand that it matters if you changed the code so that it would
> > use bdrv_reopen() in order to change the backing file, but that's not
> > what it does, is it?
> 
> 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?

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

> >> >> 2) If the 'backing' option is omitted altogether then the existing
> >> >> backing file (if any) is kept. Unlike blockdev-add this will _not_
> >> >> open the default backing file specified in the image header.
> >> >
> >> > Hm... This is certainly an inconsistency. Is it unavoidable?
> >> 
> >> I don't think we want to open new nodes on reopen(), but one easy way
> >> to avoid this behavior is simply to return an error if the user omits
> >> the 'backing' option.
> >
> > Hm, yes, not opening new nodes is a good point.
> >
> > As long as find a good solution that can be consistently applied to
> > all BlockdevRef, it should be okay. So I don't necessarily object to
> > the special casing and just leaving them as they are by default.
> >
> >> But this raises a few questions. Let's say you have an image with a
> >> backing file and you open it with blockdev-add omitting the 'backing'
> >> option. That means the default backing file is opened.
> >> 
> >>   - Do you still have to specify 'backing' on reopen? I suppose you
> >>     don't have to.
> >
> > You would have to. I agree it's ugly (not the least because you probably
> > didn't assign an explicit node name, though -drive allows specifying
> > only the node name, but still using the filename from the image file).
> 
> Yes it's a bit ugly, but we wouldn't be having a special case with the
> 'backing' option.

I think files I'm meanwhile tending towards your current solution (i.e.
default is leaving the reference as it is and you must use null to get
rid of a backing file).

> >>   - If you don't have to, can QEMU tell if bs->backing is the original
> >>     backing file or a new one? I suppose it can, by checking for
> >>     'backing / backing.*' in bs->options.
> >> 
> >>   - If you omit 'backing', does the backing file still get reopened? And
> >>     more importantly, does it keep its current options or are they
> >>     supposed to be reset to their default values?
> >
> > If you make omitting it an error, then that's not really a question?
> 
> No, if you are forced to specify 'backing' then this is not a problem.
> 
> >> >> +        /* If the 'backing' option is set and it points to a different
> >> >> +         * node then it means that we want to replace the current one,
> >> >> +         * so we shouldn't put it in the reopen queue */
> >> >> +        if (child->role == &child_backing && qdict_haskey(options, 
> >> >> "backing")) {
> >> >
> >> > I think checking child->name for "backing" makes more sense when we
> >> > also look for the "backing" key in the options QDict. This would make
> >> > generalising it for other children easier (which we probably should
> >> > do, whether in this patch or a follow-up).
> >> 
> >> Sounds reasonable.
> >> 
> >> > Do we need some way for e.g. block jobs to forbid changing the backing
> >> > file of the subchain they are operating on?
> >> 
> >> Are you thinking of any particular scenario?
> >
> > Not specifically, but I think e.g. the commit job might get confused
> > if you break its backing chain because it assumes that base is
> > somewhere in the backing chain of top, and also that it called
> > block_job_add_bdrv() on everything in the subchain it is working on.
> >
> > It just feels like we'd allow to break any such assumptions if we
> > don't block graph changes there.
> 
> 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.

Kevin