On 08.06.2016 16:38, Max Reitz wrote: > On 08.06.2016 11:32, Kevin Wolf wrote: >> Am 06.06.2016 um 16:42 hat Max Reitz geschrieben: >>> Currently, we are trying to move the backing BDS from the source to the >>> target in bdrv_replace_in_backing_chain() which is called from >>> mirror_exit(). However, mirror_complete() already tries to open the >>> target's backing chain with a call to bdrv_open_backing_file(). >>> >>> First, we should only set the target's backing BDS once. Second, the >>> mirroring block job has a better idea of what to set it to than the >>> generic code in bdrv_replace_in_backing_chain() (in fact, the latter's >>> conditions on when to move the backing BDS from source to target are not >>> really correct). >>> >>> Therefore, remove that code from bdrv_replace_in_backing_chain() and >>> leave it to mirror_complete(). >>> >>> However, mirror_complete() in turn pursues a questionable strategy by >>> employing bdrv_open_backing_file(): On the one hand, because this may >>> open the wrong backing file with drive-mirror in "existing" mode, or >>> because it will not override a possibly wrong backing file in the >>> blockdev-mirror case. >>> >>> On the other hand, we want to reuse the existing backing chain of the >>> source instead of opening everything anew, because the latter results in >>> having multiple BDSs for a single physical file and thus potentially >>> concurrent access which we should try to avoid. >> >> Careful, this "wrong" backing file might actually be intended! > > True. > > I still consider completely opening the backing chain not correct, > though, at least in absolute-paths mode, because this will result in > having at least two BDSs for single physical image files (once for the > old chain, once for the new one). > > So let's go through everything. > > == drive-mirror with absolute-paths == > > We already have the backing chain open (around the source BDS), and it's > definitely the correct one. So I think we can always reuse it for the > target. > > == drive-mirror with existing == > > You're right, we should probably keep doing bdrv_open_backing_file() > because we cannot check whether the existing image has the same backing > chain as a new absolute-paths image would have had. > > This is prone to give you some issues if you actually do want to have > the "default" backing chain, though, because of the multiple BDS thing. > This case is basically guaranteed to break with sync=none and default > image locking. > > == blockdev-mirror == > > In theory the simplest one: We just assume the backing chain of the > target has been opened already, and then we blame the user if they have > created multiple BDSs per physical file. > > Unluckily in practice, though, we require the target BDS to not have a > backing file at all. blockdev-mirror is just supposed to open the > backing chain after completion, which I really don't like (I don't think > a blockdev- command should do this kind of magic).
Good news: Turns out I was wrong. I was somehow mixing things up with blockdev-snapshot (don't ask me why, I have no clue). So I think it'd be fine to rely on the user that the backing chain of the target is correct. Max > Maybe we should allow the target to have a backing file (I really don't > see why it shouldn't have one) and treat the non-backing case like > drive-mirror in existing mode. > > > Does that sound right? > > Max > > >> Consider a case where you want to move an image with its whole backing >> chain to different storage. In that case, you would copy all of the >> backing files (cp is good enough, they are read-only), create the >> destination image which already points at the copied backing chain, and >> then mirror in "existing" mode. >> >> The intention is obviously that after the job completion the new backing >> chain is used and not the old one. >> >> I know that such cases were discussed when mirroring was introduced, I'm >> not sure whether it's actually used. We need some input there: >> >> Eric, can you tell us whether libvirt makes use of such a setup? >> >> Nir, I'm not sure who is the right person in oVirt these days, but do >> you either know yourself whether oVirt requires this to work, or do you >> know who else would know? >> >>> Thus, instead of invoking bdrv_open_backing_file(), just set the correct >>> backing BDS directly via bdrv_set_backing_hd(). Also, do so only when >>> mirror_complete() is certain to succeed. >>> >>> In contrast to what bdrv_replace_in_backing_chain() did so far, we do >>> not need to drop the source's backing file. >>> >>> Signed-off-by: Max Reitz <mre...@redhat.com> >> >> Leaving the actual code review for later when we have decided what >> semantics we even want. >> >> Kevin >> > >
signature.asc
Description: OpenPGP digital signature
