Re: [PATCH RFC v2 1/5] block: add bitmap-populate job

[Peter Krempa]

On Mon, Jun 08, 2020 at 13:30:48 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2020 12:38, Peter Krempa wrote:
> > On Sat, Jun 06, 2020 at 09:55:13 +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > 05.06.2020 13:59, Peter Krempa wrote:

[...]

> > 
> > It's not only a "user forgot" thing, but more that a systemic change
> > would be required.
> > 
> > Additionally until _very_ recently it wasn't possible to create bitmaps
> > using qemu-img, which made it impossible to create overlays for inactive
> 
> Didn't you consider to use qemu started in stopped mode to do block
> operations in same manner as for running vm? What's wrong with it?
> Also, there is qemu-storage-daemon, which is developed as separated
> binary, where block-layer is compiled in together with QMP interface.

It's still considered, but I didn't have time to implement anything
related to it and nobody else implemented it either.

Additionally the problem isn't in libvirt's handling but more in other
apps handling. We still due to historical reasons support if users
create overlays outside of libvirt.

This would mean that either backups break if the overlay is not done
properly or we have to have a fallback to use
'block-dirty-bitmap-populate'. At this point for both my sanity and
actually finishing all the details in regards to incremental backup

> > VMs. Arguably this has changed so we could require it. It still adds a
> > moving part which can break if the user doesn't add the bitmap or
> > requires yet another special case handling if we want to compensate for
> > that.
> > 
> > As of such, in libvirt's tech-preview implementation that is present
> > currently upstream, if you create a qcow2 overlay without adding the
> > appropriate bitmaps, the backups simply won't work.
> > 
> > > What do you think of granularity? We in Virtuozzo use 1M cluster as a 
> > > default for qcow2 images. But we use 64k granularity (default) for 
> > > bitmaps, to have smaller incremental backups. So, this is advantage of 
> > > creating bitmap over relaying on block-status capturing by 
> > > block-dirty-bitmap-populate: you don't control dirtiness granularity. So, 
> > > I think that bitmap propagation, or just creating new dirty bitmap to 
> > > track dirtiness from start of new snapshot is better.
> > 
> > This is a valid argument. Backups in this scenario will be bigger. I
> > still don't feel like the code needs to be made much more complex
> > because of it though.
> 
> May be, there is a simple solution? an option for blockdev-snapshot-sync to 
> create a bitmap in a new image (or if you create image by qemu-img, just 
> create bitmap by qemu-img as well, using new functionality).

You mean like we do now?:

https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_driver.c#L15020

That is slated to be deleted with my patchset though.

Good thing is that we can theoretically re-add it later.

For now I'd go with the simpler option that has fewer side effects.

> Isn't it simpler than to just use existing block-status-bitmap, than run a 
> job?

No. Because we'd still need to support cases where user added an overlay
without the appropriate bitmap. As said I'd prefer to keep the code
simple and then work on optimizations. Good thing is that with 'one
active bitmap per point in time' this is simpler to achieve.

[...]

> > > 
> > > What is 'merge' step?
> > 
> > In some previous replies to Kevin, we discussed that it might be worth
> > optimizing 'block-dirty-bitmap-populate' to directly populate the bits
> > in the target bitmap rather than after the job is complete, so
> > efectively directly mering it. I probably described it wrong here.
> > 
> > > Do you mean that populating directly into target bitmap is not really 
> > > needed?
> > 
> > I need the bitmap populated by 'block-dirty-bitmap-populate' to be
> > merged into multiple bitmaps in the new semantics. If the job itself
> > doesn't support that sematics, changing it to just to directly populate
> > one bitmap doesn't seem to be worth it since I'll be using intermediate
> > bitmaps anyways.
> > 
> 
> Hmm, if the main use case of populating job is to merge changes since 
> snapshot to several bitmaps (all active bitmaps?), than I think it's correct 
> to implement exactly this semantics, allowing a list of targets as well as 
> list of source bitmaps. We even can reuse same structure for target-list 
> which we use for source-list. And it's simple to implement in Qemu.

I'd mainly like for the design to converge. I actually have almost
complete patches which use the current semantics. I'm okay with
reworking it but at some point there should be a line when it won't
change.