Re: x-blockdev-reopen & block-dirty-bitmaps

[Kevin Wolf]

Am 18.02.2020 um 13:58 hat Peter Krempa geschrieben:
> On Mon, Feb 17, 2020 at 10:52:31 +0100, Kevin Wolf wrote:
> > Am 14.02.2020 um 21:32 hat John Snow geschrieben:
> > > On 2/14/20 3:19 PM, Kevin Wolf wrote:
> > > > Am 14.02.2020 um 19:54 hat John Snow geschrieben:
> > > >> Hi, what work remains to make this a stable interface, is it known?
> > > >>
> > > >> We're having a problem with bitmaps where in some cases libvirt wants 
> > > >> to
> > > >> commit an image back down to a base image but -- for various reasons --
> > > >> the bitmap was left enabled in the backing image, so it would accrue 
> > > >> new
> > > >> writes during the commit.
> > > >>
> > > >> Normally, when creating a snapshot using blockdev-snapshot, the backing
> > > >> file becomes RO and all of the bitmaps become RO too.
> > > >>
> > > >> The goal is to be able to disable (or enable) bitmaps from a backing
> > > >> file before (or atomically just before) a commit operation to allow
> > > >> libvirt greater control on snapshot commit.
> > > >>
> > > >> Now, in my own testing, we can reopen a backing file just fine, delete
> > > >> or disable a bitmap and be done with it -- but the interface isn't
> > > >> stable, so libvirt will be reluctant to use such tricks.
> 
> Well, while we probably want it to be stable for upstream acceptance
> that didn't prevent me from actually trying to use reopening. It would
> be probably frowned upon if I tried to use it upstream.
> 
> The problem is that we'd have to carry the compatibility code for at
> least the two possible names of the command if nothing else changes and
> also the fact that once the command is declared stable, some older
> libvirt versions might not know to use it.

I think this is exactly the thing we need before we can mark it stable:
Some evidence that it actually provides the functionality that
management tools need. So thanks for giving it a try.

> The implementation was surprisingly easy though and works well to reopen
> the backing files in RW mode. The caveat was that the reopen somehow
> still didn't reopen the bitmaps and qemu ended up reporting:
> 
> libvirt-1-format: Failed to make dirty bitmaps writable: Cannot update bitmap 
> directory: Bad file descriptor
> 
> So unfortunately it didn't work out for that scenario.

I'm not completely sure, but this sounds a bit like a reopen bug in the
file-posix driver to me, where we keep using the old file descriptor
somewhere?

Someone (TM) should turn this into a qemu-iotests case and then we can
debug it.

> <sidetrack alert>
> 
> Also I'm afraid I have another use case for it:
> 
> oVirt when doing their 'live storage migration' actually uses libvirt to
> mirror only the top layer in shallow mode and copies everything else
> while the mirror is running using qemu-img.
> 
> Prior to libvirt's use of -blockdev this worked well, because qemu
> reopened the mirror destination (which caused to open the backing files)
> only at the end. With -blockdev we have to open the backing files right
> away so that they can be properly installed as backing of the image
> being mirrored and oVirt's qemu-img instance gets a locking error as the
> images are actually opened for reading already.
> 
> I'm afraid that we won't be able to restore the previous semantics
> without actually opening the backing files after the copy is
> synchronized before completing the job and then installing them as the
> backing via blockdev-reopen.
> 
> Libvirt's documentation was partially alibistic [1] and asked the user to
> actually provide a working image but oVirt actually exploited the qemu
> behaviour to allow folding the two operations together.
> 
> [1] https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainBlockCopy

This sounds like a case that blockdev-snapshot might be able to solve:
After the offline copy has completed, you blockdev-add the whole backing
chain for the target and then use blockdev-snapshot to add the active
layer (that had 'backing': null) to it.

> > > >>
> > > >> Probably a loaded question, but:
> > > >>
> > > >> - What's needed to make the interface stable?
> > > >> - Are there known problem points?
> > > >> - Any suggestions for workarounds in the meantime?
> > > > 
> > > > I think I've asked this before, but I don't remember the answer...
> > > > 
> > > > What would be the problem with letting the enable/disable command
> > > > temporarily reopen the backing file read-write, like the commit job [1]
> > > > does?
> > > > 
> > > 
> > > I guess no problem? I wouldn't want it to do this automatically, but
> > > perhaps we could add a "force=True" bool where it tries to do just this.
> > > 
> > > (And once reopen works properly we could deprecate this workaround again.)
> > 
> > I'm not sure if I would view this only as a workaround, but if you like
> > it better with force=true, I won't object either.
> 
> I'm wondering whether adding a feature deprecating it soon is even worth
> doing. From libvirt's point of view it would be comparable to using the
> x-prefixed command, with just slightly longer transition period. (
> deleting the x-prefix is a very hard transition to cope with)

It would only make sense if we made it work that way as a permanent
solution.

Kevin