Re: [Qemu-devel] [Qemu-block] [PATCH 11/34] block: Allow references for backing files

[Kevin Wolf]

Am 27.05.2015 um 14:31 hat Kevin Wolf geschrieben:
> Am 21.05.2015 um 07:47 hat Wen Congyang geschrieben:
> > On 05/09/2015 01:21 AM, Kevin Wolf wrote:
> > > For bs->file, using references to existing BDSes has been possible for a
> > > while already. This patch enables the same for bs->backing_hd.
> > 
> > 1. We reference to an existing BDSes, and some disk uses this blk. Do
> > we allow this?
> 
> Currently yes. If it breaks, you get to keep both pieces.
> 
> As long as your guest device is read-only, it should just work. It would
> be a very bad idea, though, to write to a backing file.
> 
> Op blockers should eventually prevent this from happening (Jeff, you may
> want to take a note ;-))
> 
> > 2. bs->backing_hd->blk can be not NULL now? If we do an active commit
> > to this backing file(use mirror job), we will call bdrv_swap() in
> > mirror_exit(), and the function bdrv_swap() doesn't allow that
> > new_bs->blk(here is bs->backing_hd) is not NULL.
> 
> You're right.
> 
> I can remove this patch from the series for now, but of course that
> doesn't solve the problem.

On second thought, I don't want to remove the patch from the series
because then I don't have test cases for backing files that don't
inherit option from their parents. (Also, it would lead to some
conflicts through the series.)

> I'm not sure what to do about it. Making
> bdrv_swap() work with BDSes that have BB attached is probably another
> item in the list of "dynamic reconfiguration" problems.
> 
> Markus, any ideas?

So it would be even more important to figure out what to do with
bdrv_swap().

Paolo, you added that bunch of assertions to bdrv_swap() when you
introduced it in commit 4ddc07ca. Did you just add them because they
were true at the time, or is anything actually relying on these
invariants?

What do we actually want to happen? Let's assume a small chain of
backing files:

        +---------- A-BlockBackend
        |
        |    +----- B-BlockBackend
        |    |
base <- A <- B

After completing the commit operation, what we want to have for the BB
of B is clear:

        +---------- B-BlockBackend
        |
base <- A

If we just remove the assertions that are currently present in
bdrv_swap(), I think we'd end up with this:

             +----- A-BlockBackend
             |
        +---------- B-BlockBackend
        |    |
base <- A <- B

This is probably surprising for the user if they ever look at
A-BlockBackend again. It's also surprising because (unlike the case
without a BB for A) B is actually still referenced and therefore the
file stays opened.

I suspect what we really want is this (which is not swapping):

        +---------- A-BlockBackend
        |
        +---------- B-BlockBackend
        |
base <- A

Both BBs point to the same BDS now and B is actually closed.

Any opinions on this?

Kevin