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

[Kevin Wolf]

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.
> >>
> >> 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.

> > [1] I mean, I know that this code is wrong strictly speaking because we
> >     really should be counting read-write users [2] rather than blindly
> >     making the node read-only at the end of the operation - but somehow
> >     it seems to work in practice for commit jobs.
> > 
> > [2] Counting really means just looking at parent BdrvChild links that
> >     have WRITE permissions. I guess doing it right would mean getting
> >     rid of BlockDriverState.read_only (which is redundant) and using
> >     only permissions.
> 
> OK, sounds good. I'll make a mockup that tries to accurately detect
> read-only-ness and reverts changes only if it made any to begin with.

Fixing it for commit would be appreciated, though as I said it seems to
be a theoretical case mostly because we never got bug reports for it.

For bitmaps it's even more theoretical because we hold the BQL between
the switch to read-write and the switch back. So I don't think we can
actually run into this case for the bitmap enable/disable operation.

Kevin