Re: [Qemu-devel] [PATCH v15 18/21] block: Reuse bs as backing hd for drive-backup sync=none

[Fam Zheng]

On Wed, 04/26 16:34, Kevin Wolf wrote:
> Am 26.04.2017 um 15:15 hat Fam Zheng geschrieben:
> > On Wed, 04/26 14:52, Kevin Wolf wrote:
> > > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > > Signed-off-by: Fam Zheng <address@hidden>
> > > 
> > > The commit message is a bit terse. :-)
> > > 
> > > So I think this means that instead of opening the backing file of the
> > > backup (which is probably the active file of the VM) a second time, we
> > > instead take a reference on the existing node. Very good idea.
> > > 
> > > In fact, my question is: How did this ever work? Did we just neglect to
> > > test this? The backup backing file will use stale qcow2 metadata when we
> > > continue to write to the source.
> > 
> > Reading from the target BDS would be a problem, but I guess it's relatively
> > uncommon:
> > 
> > - In QEMU code, COW is always done manually in backup_do_cow, so no reading 
> > from
> >   target->backing in block layer.
> > 
> > - Writing to target is done in target cluster granularity so no COW too.
> > 
> > But it's still a fully valid point, for example it can be exported to NBD, 
> > etc.
> 
> Which I believe is the exact setup for fleecing.

Hmm, yes you must be right. Though IMO I've always considered blockdev-add with
backing reference to be the only valid way to do fleecing, this one is IMHO a
hack. And when it was added, there was no way to export it in NBD because it
didn't have a device id.

> 
> And actually, if we were sure that we never read from the file, we
> wouldn't even have to attach the backing file in the first place.

OK, fair enough.

> 
> > > Unless I'm missing something, I'd propose this for qemu-stable.
> > 
> > Yes, sounds good.
> > 
> > > 
> > > Also, mirror with MIRROR_OPEN_BACKING_CHAIN probably has a similar
> > > problem with opening the images in the backing chain a second time.
> > 
> > Seems so. But if there was such a test case, image locking would have
> > complained.
> > 
> > Is that a practical use case?  Mirror target image uses the active image as
> > backign file? (It's a bit late in the evening, please remind me :-)
> 
> I believe storage migration uses a mode like this, but to be honest I
> don't remember the details at the moment either.
> 
> But I do see that NEW_IMAGE_MODE_ABSOLUTE_PATHS enters the source as the
> backing file of the target and opens it (a second instance of it) when
> the mirror completes. Hopefully, by that time the source doesn't have
> another user that keeps it alive (and I think op blockers should
> actually ensure this), but it looks a bit fishy. Probably worth a closer
> look.

Mirror has many graph manipulation magic, which seems even darker than commit.
We should perhaps allocate sometime to see if there are cleaner ways to do them,
with all the new infrastructure we have. Hopefully it's not hard to do, like
this one.

Fam