[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Kevin Wolf
Subject: Re: x-blockdev-reopen & block-dirty-bitmaps
Date: Mon, 17 Feb 2020 10:52:31 +0100
User-agent: Mutt/1.12.1 (2019-06-15)

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]