On 27/05/2015 15:30, Kevin Wolf wrote: > 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?
When I added bdrv_swap, the idea was to bs_new was either being closed afterwards, or (for bdrv_append) it would be the backing file of bs_top. In either case, the assertions would make sure that nothing would break when further manipulating bs_new. Are they being relied on? Probably not, but some violations would be just weird, such as a device attached to a non-topmost BDS, or a job attached to the old version of a mirrored disk. > 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. Correct. Swapping was just a trick to do things underneath the device's feet, effectively emulating BlockBackends. Since I'm not very much up-to-date with the block device graph stuff, do BDSes have a list of BlockBackends that point to them, so that they can redirect all those BlockBackends to the backing file? Paolo
