[Qemu-devel] BdrvDirtyBitmap and bdrv_snapshot_goto

[Qemu-devel] BdrvDirtyBitmap and bdrv_snapshot_goto

[Vladimir Sementsov-Ogievskiy]

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?

Also, when we will have persistent bitmaps in qcow2, haw they should be 
handled on snapshot switching?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] BdrvDirtyBitmap and bdrv_snapshot_goto

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

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

> Also, when we will have persistent bitmaps in qcow2, haw they should be
> handled on snapshot switching?

Good question. Since persistent bitmaps are not bound to snapshots, I'd
fill them with ones for now, too.

It would probably make sense to bind bitmaps to snapshots, though. This
could be achieved by adding a bitmap directory pointer to each snapshot
table entry. When switching snapshots, software (i.e. qemu) could then
either:

(1) Fill the bitmaps with ones, thus treating them as "global" bitmaps.

(2) Save the current bitmap directory in the old snapshot and put the
one from the snapshot that is being switched to into the image header,
thus treating them as bound to the snapshot.

Of course, this could be a bitmap-specific property.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] BdrvDirtyBitmap and bdrv_snapshot_goto

[Vladimir Sementsov-Ogievskiy]

07.11.2016 19:10, Max Reitz wrote:
> 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...

Using block jobs is not necessary - we just have to maintain our dirty 
bitmap while qemu works, regardless of block jobs.

>
>> Also, when we will have persistent bitmaps in qcow2, haw they should be
>> handled on snapshot switching?
> Good question. Since persistent bitmaps are not bound to snapshots, I'd
> fill them with ones for now, too.
>
> It would probably make sense to bind bitmaps to snapshots, though. This
> could be achieved by adding a bitmap directory pointer to each snapshot
> table entry. When switching snapshots, software (i.e. qemu) could then
> either:
>
> (1) Fill the bitmaps with ones, thus treating them as "global" bitmaps.
>
> (2) Save the current bitmap directory in the old snapshot and put the
> one from the snapshot that is being switched to into the image header,
> thus treating them as bound to the snapshot.
>
> Of course, this could be a bitmap-specific property.
>
> Max
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] BdrvDirtyBitmap and bdrv_snapshot_goto

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

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.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] BdrvDirtyBitmap and bdrv_snapshot_goto

[Vladimir Sementsov-Ogievskiy]

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

In other words, dirty bitmaps should be deleted on snapshot switch? All? 
Or only named?


-- 
Best regards,
Vladimir

Re: [Qemu-devel] BdrvDirtyBitmap and bdrv_snapshot_goto

[Kevin Wolf]

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.

Kevin

Re: [Qemu-devel] BdrvDirtyBitmap and bdrv_snapshot_goto

[Vladimir Sementsov-Ogievskiy]

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

>
> Kevin


-- 
Best regards,
Vladimir

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