Re: [Qemu-devel] BdrvDirtyBitmap and bdrv_snapshot_goto

[Kevin Wolf]

Am 08.11.2016 um 17:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 08.11.2016 15:18, Kevin Wolf wrote:
> >Am 08.11.2016 um 12:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>08.11.2016 14:05, Kevin Wolf wrote:
> >>>Am 07.11.2016 um 17:10 hat Max Reitz geschrieben:
> >>>>On 07.11.2016 16:24, Vladimir Sementsov-Ogievskiy wrote:
> >>>>>Hi all!
> >>>>>
> >>>>>As I can see, in bdrv_snapshot_goto, existing dirty bitmaps are not
> >>>>>handled.. Is it ok? Should not they be filled with ones or something
> >>>>>like this?
> >>>>Filling them with ones makes sense to me. I guess nobody noticed because
> >>>>nobody was crazy enough to use block jobs alongside loadvm...
> >>>What's the use case in which ones make sense?
> >>>
> >>>It rather seems to me that an active dirty bitmap and snapshot switching
> >>>should exclude each other because the bitmap becomes meaningless by the
> >>>switch. And chances are that after switching a snapshot you don't want
> >>>to "incrementally" backup everything, but that you should access a
> >>>different backup.
> >>In other words, dirty bitmaps should be deleted on snapshot switch?
> >>All? Or only named?
> >As Max said, we should probably integrate bitmaps with snapshots. After
> >reloading the old state, the bitmap becomes valid again, so throwing it
> >away in the active state seems only right if we included it in the
> >snapshot and can bring it back.
> 
> If we choose this way, it should be firstly done for
> BdrvDirtyBitmap's without any persistance. And it is not as simple
> as just drop dirty bitmaps or fill them with ones. Current behavior
> is definitely wrong: if user create incremental backup after
> snapshot switch this incremental backup will be incorrect. I think
> it should be fixed now simpler way (actually this fix means "for now
> incremental backup is incompatible with snapshot switch"), and in
> future, if we really need this, make them work together.
> 
> Also, I think that filling with ones is safer and more native. It
> really describes, what happens (with some overhead of dirty bits).

No, it doesn't. Loading a snapshot really means accessing a different
image, even if that different image is stored in the same file. So if
you load a snapshot and keep using the same bitmap, you're now using a
bitmap that describes a different image, and of course that's wrong.

But writing ones there is just as wrong. You're treating a changed
image the same way as if the same original image had been completely
overwritten. But that's not what happened.

We just need to take care that every bitmap is always used with the
image that it really describes. And if we can't get this right yet
across snapshot loads, we have to forbid using both together.

I suspect we also have a similar problem with removable media. Which
probably means that you can't use removable media and bitmaps together.

> Simple improvement: instead of filling with ones,
> new_dirty_bitmap_state  = old_dirty_bitmap_state |
> old_allocated_mask | new_allocated_mask, where allocated mask is
> bitmap with same granularity, showing which ranges are allocated in
> the image.

Optimising an approach that doesn't make sense is a waste of time.

Kevin