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
