[Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Vladimir Sementsov-Ogievskiy]

Snapshot-switch actually changes active state of disk so it should
reflect on dirty bitmaps. Otherwise next incremental backup using
these bitmaps will be invalid.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/snapshot.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/block/snapshot.c b/block/snapshot.c
index a46564e7b7..1d5ab5f90f 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -181,10 +181,24 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
 {
     BlockDriver *drv = bs->drv;
     int ret, open_ret;
+    int64_t len;
 
     if (!drv) {
         return -ENOMEDIUM;
     }
+
+    len = bdrv_getlength(bs);
+    if (len < 0) {
+        return len;
+    }
+    /* We should set all bits in all enabled dirty bitmaps, because dirty
+     * bitmaps reflect active state of disk and snapshot switch operation
+     * actually dirties active state.
+     * TODO: It may make sense not to set all bits but analyze block status of
+     * current state and destination snapshot and do not set bits corresponding
+     * to both-zero or both-unallocated areas. */
+    bdrv_set_dirty(bs, 0, len);
+
     if (drv->bdrv_snapshot_goto) {
         return drv->bdrv_snapshot_goto(bs, snapshot_id);
     }
-- 
2.11.1

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Eric Blake]

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

On 10/23/2017 11:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> Snapshot-switch actually changes active state of disk so it should
> reflect on dirty bitmaps. Otherwise next incremental backup using
> these bitmaps will be invalid.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/snapshot.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[John Snow]

On 10/23/2017 05:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> Snapshot-switch actually changes active state of disk so it should
> reflect on dirty bitmaps. Otherwise next incremental backup using
> these bitmaps will be invalid.
> 

Good call. I knew that snapshots weren't compatible, but this makes it
more obvious and less error-prone to an end user.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
>  block/snapshot.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index a46564e7b7..1d5ab5f90f 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -181,10 +181,24 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>  {
>      BlockDriver *drv = bs->drv;
>      int ret, open_ret;
> +    int64_t len;
>  
>      if (!drv) {
>          return -ENOMEDIUM;
>      }
> +
> +    len = bdrv_getlength(bs);
> +    if (len < 0) {
> +        return len;
> +    }
> +    /* We should set all bits in all enabled dirty bitmaps, because dirty
> +     * bitmaps reflect active state of disk and snapshot switch operation
> +     * actually dirties active state.
> +     * TODO: It may make sense not to set all bits but analyze block status of
> +     * current state and destination snapshot and do not set bits corresponding
> +     * to both-zero or both-unallocated areas. */
> +    bdrv_set_dirty(bs, 0, len);
> +
>      if (drv->bdrv_snapshot_goto) {
>          return drv->bdrv_snapshot_goto(bs, snapshot_id);
>      }
> 

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Max Reitz]

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

On 2017-10-23 11:29, Vladimir Sementsov-Ogievskiy wrote:
> Snapshot-switch actually changes active state of disk so it should
> reflect on dirty bitmaps. Otherwise next incremental backup using
> these bitmaps will be invalid.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/snapshot.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Thanks, applied to my block tree:

https://github.com/XanClic/qemu/commits/block

Max


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

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Kevin Wolf]

Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Snapshot-switch actually changes active state of disk so it should
> reflect on dirty bitmaps. Otherwise next incremental backup using
> these bitmaps will be invalid.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We discussed this quite a while ago, and I'm still not convinced that
this approach makes sense.

Can you give just one example of a use case where dirtying the whole
bitmap while loading a snapshot is the desired behaviour?

I think the most useful behaviour would be something where the bitmaps
themselves are snapshotted, too. But for the time being, the easiest and
safest solution might just be to error out in any snapshot operations
if any bitmaps are in use.

Kevin

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Denis V. Lunev]

On 11/17/2017 03:30 PM, Kevin Wolf wrote:
> Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Snapshot-switch actually changes active state of disk so it should
>> reflect on dirty bitmaps. Otherwise next incremental backup using
>> these bitmaps will be invalid.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> We discussed this quite a while ago, and I'm still not convinced that
> this approach makes sense.
>
> Can you give just one example of a use case where dirtying the whole
> bitmap while loading a snapshot is the desired behaviour?
>
> I think the most useful behaviour would be something where the bitmaps
> themselves are snapshotted, too. But for the time being, the easiest and
> safest solution might just be to error out in any snapshot operations
> if any bitmaps are in use.
>
> Kevin
The problem is that snapshotting of bitmaps will just provide wrong result.

Let us assume that we have bitmap named A.

The user has started it and made full backup B.
The user made snapshot S. At this moment bitmap A is saved as A' to bitmap.
The user has made incremental backup B1. A is reset to 0.
The user has made incremental backup B2. A is reset to 0 again.

At this moment the user has reverted to snapshot S.
What we need to make incremental backup at the moment?
The difference in between states B2 and S. This is __for sure__
not A'. Thus saving of the bitmap at the moment is quite
useless and we need to reset bitmap to full.

Den

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Kevin Wolf]

Am 17.11.2017 um 13:58 hat Denis V. Lunev geschrieben:
> On 11/17/2017 03:30 PM, Kevin Wolf wrote:
> > Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> Snapshot-switch actually changes active state of disk so it should
> >> reflect on dirty bitmaps. Otherwise next incremental backup using
> >> these bitmaps will be invalid.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > We discussed this quite a while ago, and I'm still not convinced that
> > this approach makes sense.
> >
> > Can you give just one example of a use case where dirtying the whole
> > bitmap while loading a snapshot is the desired behaviour?
> >
> > I think the most useful behaviour would be something where the bitmaps
> > themselves are snapshotted, too. But for the time being, the easiest and
> > safest solution might just be to error out in any snapshot operations
> > if any bitmaps are in use.
> >
> The problem is that snapshotting of bitmaps will just provide wrong
> result.

Only if the user does the wrong thing. :-)

> Let us assume that we have bitmap named A.
> 
> The user has started it and made full backup B.
> The user made snapshot S. At this moment bitmap A is saved as A' to bitmap.
> The user has made incremental backup B1. A is reset to 0.
> The user has made incremental backup B2. A is reset to 0 again.
> 
> At this moment the user has reverted to snapshot S.
> What we need to make incremental backup at the moment?

The important point here is that the backup that you should make is not
incremental compared to B2, but to B (i.e. the last backup at the point
when the snapshot was made).

If your snapshot chain branches, your backups have to branch, too.

> The difference in between states B2 and S. This is __for sure__
> not A'. Thus saving of the bitmap at the moment is quite
> useless and we need to reset bitmap to full.

A' still contains the differences between B and the point when S was
created. So if you take a new incremental backup B'1 based on B, the
correct bitmap to use would be A'.

But I agree that this is complicated and probably easy to misuse, so
just erroring out is a serious option. And if we do this, at least we
don't set a bad solution in stone and can always add a better solution
if we can later think of one.

I just don't think that marking everything dirty is a good solution. It
forces the user to make a full backup, so we could just as well ask the
user to delete the bitmap before they load a snapshot.

Kevin

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Denis V. Lunev]

On 11/17/2017 04:42 PM, Kevin Wolf wrote:
> Am 17.11.2017 um 13:58 hat Denis V. Lunev geschrieben:
>> On 11/17/2017 03:30 PM, Kevin Wolf wrote:
>>> Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> Snapshot-switch actually changes active state of disk so it should
>>>> reflect on dirty bitmaps. Otherwise next incremental backup using
>>>> these bitmaps will be invalid.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> We discussed this quite a while ago, and I'm still not convinced that
>>> this approach makes sense.
>>>
>>> Can you give just one example of a use case where dirtying the whole
>>> bitmap while loading a snapshot is the desired behaviour?
>>>
>>> I think the most useful behaviour would be something where the bitmaps
>>> themselves are snapshotted, too. But for the time being, the easiest and
>>> safest solution might just be to error out in any snapshot operations
>>> if any bitmaps are in use.
>>>
>> The problem is that snapshotting of bitmaps will just provide wrong
>> result.
> Only if the user does the wrong thing. :-)
>
>> Let us assume that we have bitmap named A.
>>
>> The user has started it and made full backup B.
>> The user made snapshot S. At this moment bitmap A is saved as A' to bitmap.
>> The user has made incremental backup B1. A is reset to 0.
>> The user has made incremental backup B2. A is reset to 0 again.
>>
>> At this moment the user has reverted to snapshot S.
>> What we need to make incremental backup at the moment?
> The important point here is that the backup that you should make is not
> incremental compared to B2, but to B (i.e. the last backup at the point
> when the snapshot was made).
>
> If your snapshot chain branches, your backups have to branch, too.
>
>> The difference in between states B2 and S. This is __for sure__
>> not A'. Thus saving of the bitmap at the moment is quite
>> useless and we need to reset bitmap to full.
> A' still contains the differences between B and the point when S was
> created. So if you take a new incremental backup B'1 based on B, the
> correct bitmap to use would be A'.
>
> But I agree that this is complicated and probably easy to misuse, so
> just erroring out is a serious option. And if we do this, at least we
> don't set a bad solution in stone and can always add a better solution
> if we can later think of one.
>
> I just don't think that marking everything dirty is a good solution. It
> forces the user to make a full backup, so we could just as well ask the
> user to delete the bitmap before they load a snapshot.
>
> Kevin
Agree. This is also an option.

Den

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Max Reitz]

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

On 2017-11-17 13:30, Kevin Wolf wrote:
> Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Snapshot-switch actually changes active state of disk so it should
>> reflect on dirty bitmaps. Otherwise next incremental backup using
>> these bitmaps will be invalid.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> We discussed this quite a while ago, and I'm still not convinced that
> this approach makes sense.

I think it at least makes more sense than not handling this case at all.

> Can you give just one example of a use case where dirtying the whole
> bitmap while loading a snapshot is the desired behaviour?
> 
> I think the most useful behaviour would be something where the bitmaps
> themselves are snapshotted, too.

Agreed.

>                                  But for the time being, the easiest and
> safest solution might just be to error out in any snapshot operations
> if any bitmaps are in use.

Sounds OK, too.  I personally don't have an opinion either way.

But in any case, what we did before this patch was definitely wrong so I
consider it an improvement.

Max


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

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[John Snow]

On 11/17/2017 10:01 AM, Max Reitz wrote:
> On 2017-11-17 13:30, Kevin Wolf wrote:
>> Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> Snapshot-switch actually changes active state of disk so it should
>>> reflect on dirty bitmaps. Otherwise next incremental backup using
>>> these bitmaps will be invalid.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> We discussed this quite a while ago, and I'm still not convinced that
>> this approach makes sense.
> 
> I think it at least makes more sense than not handling this case at all.
> 
>> Can you give just one example of a use case where dirtying the whole
>> bitmap while loading a snapshot is the desired behaviour?
>>
>> I think the most useful behaviour would be something where the bitmaps
>> themselves are snapshotted, too.
> 
> Agreed.
> 
>>                                  But for the time being, the easiest and
>> safest solution might just be to error out in any snapshot operations
>> if any bitmaps are in use.
> 
> Sounds OK, too.  I personally don't have an opinion either way.
> 
> But in any case, what we did before this patch was definitely wrong so I
> consider it an improvement.
> 

This is how I feel about it too. Erroring out entirely is an option, but
code-wise just dirtying everything is at least verifiably not-wrong and
pretty simple to implement.

It's an improvement... Don't do it, but at least you won't get something
wrong after, just something heinously unoptimal.

> Max
> 

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Kevin Wolf]

Am 17.11.2017 um 19:15 hat John Snow geschrieben:
> 
> 
> On 11/17/2017 10:01 AM, Max Reitz wrote:
> > On 2017-11-17 13:30, Kevin Wolf wrote:
> >> Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>> Snapshot-switch actually changes active state of disk so it should
> >>> reflect on dirty bitmaps. Otherwise next incremental backup using
> >>> these bitmaps will be invalid.
> >>>
> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>
> >> We discussed this quite a while ago, and I'm still not convinced that
> >> this approach makes sense.
> > 
> > I think it at least makes more sense than not handling this case at all.
> > 
> >> Can you give just one example of a use case where dirtying the whole
> >> bitmap while loading a snapshot is the desired behaviour?
> >>
> >> I think the most useful behaviour would be something where the bitmaps
> >> themselves are snapshotted, too.
> > 
> > Agreed.
> > 
> >>                                  But for the time being, the easiest and
> >> safest solution might just be to error out in any snapshot operations
> >> if any bitmaps are in use.
> > 
> > Sounds OK, too.  I personally don't have an opinion either way.
> > 
> > But in any case, what we did before this patch was definitely wrong so I
> > consider it an improvement.
> > 
> 
> This is how I feel about it too. Erroring out entirely is an option, but
> code-wise just dirtying everything is at least verifiably not-wrong and
> pretty simple to implement.

But that's exactly the problem: If something is just plain wrong, you
can always replace it with something that makes sense. If it errors out,
you can still remove that error later. But if you have something that
doesn't make a whole lot of sense, but kinda sorta works (like after
this patch), it's ABI and you can't implement something more useful
later any more.

> It's an improvement... Don't do it, but at least you won't get
> something wrong after, just something heinously unoptimal.

It's a short-term improvement that may become a long-term burden.

Kevin

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[John Snow]

On 11/17/2017 01:25 PM, Kevin Wolf wrote:
> Am 17.11.2017 um 19:15 hat John Snow geschrieben:
>>
>>
>> On 11/17/2017 10:01 AM, Max Reitz wrote:
>>> On 2017-11-17 13:30, Kevin Wolf wrote:
>>>> Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> Snapshot-switch actually changes active state of disk so it should
>>>>> reflect on dirty bitmaps. Otherwise next incremental backup using
>>>>> these bitmaps will be invalid.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>
>>>> We discussed this quite a while ago, and I'm still not convinced that
>>>> this approach makes sense.
>>>
>>> I think it at least makes more sense than not handling this case at all.
>>>
>>>> Can you give just one example of a use case where dirtying the whole
>>>> bitmap while loading a snapshot is the desired behaviour?
>>>>
>>>> I think the most useful behaviour would be something where the bitmaps
>>>> themselves are snapshotted, too.
>>>
>>> Agreed.
>>>
>>>>                                  But for the time being, the easiest and
>>>> safest solution might just be to error out in any snapshot operations
>>>> if any bitmaps are in use.
>>>
>>> Sounds OK, too.  I personally don't have an opinion either way.
>>>
>>> But in any case, what we did before this patch was definitely wrong so I
>>> consider it an improvement.
>>>
>>
>> This is how I feel about it too. Erroring out entirely is an option, but
>> code-wise just dirtying everything is at least verifiably not-wrong and
>> pretty simple to implement.
> 
> But that's exactly the problem: If something is just plain wrong, you
> can always replace it with something that makes sense. If it errors out,
> you can still remove that error later. But if you have something that
> doesn't make a whole lot of sense, but kinda sorta works (like after
> this patch), it's ABI and you can't implement something more useful
> later any more.
> 
>> It's an improvement... Don't do it, but at least you won't get
>> something wrong after, just something heinously unoptimal.
> 
> It's a short-term improvement that may become a long-term burden.
> 
> Kevin
> 

I see your point.

If we enable it now, we always have to.
If we disable it now, we *can* later if we wish.

...however, I think it's been the case that we haven't prohibited it
before, but also it's a pretty good case that nobody has been using this
feature in production because they're not yet persistent and migratable.

An error message asking the user to delete bitmaps (which will very
obviously invalidate them) would be fine, too. I was erring on the side
of "just let things work," but you have a point that making sure the
user knows that what they're trying to accomplish is not a good idea is
probably better than silently doing the very stupid thing.

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Vladimir Sementsov-Ogievskiy]

17.11.2017 23:40, John Snow wrote:
>
> On 11/17/2017 01:25 PM, Kevin Wolf wrote:
>> Am 17.11.2017 um 19:15 hat John Snow geschrieben:
>>>
>>> On 11/17/2017 10:01 AM, Max Reitz wrote:
>>>> On 2017-11-17 13:30, Kevin Wolf wrote:
>>>>> Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> Snapshot-switch actually changes active state of disk so it should
>>>>>> reflect on dirty bitmaps. Otherwise next incremental backup using
>>>>>> these bitmaps will be invalid.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> We discussed this quite a while ago, and I'm still not convinced that
>>>>> this approach makes sense.
>>>> I think it at least makes more sense than not handling this case at all.
>>>>
>>>>> Can you give just one example of a use case where dirtying the whole
>>>>> bitmap while loading a snapshot is the desired behaviour?
>>>>>
>>>>> I think the most useful behaviour would be something where the bitmaps
>>>>> themselves are snapshotted, too.
>>>> Agreed.
>>>>
>>>>>                                   But for the time being, the easiest and
>>>>> safest solution might just be to error out in any snapshot operations
>>>>> if any bitmaps are in use.
>>>> Sounds OK, too.  I personally don't have an opinion either way.
>>>>
>>>> But in any case, what we did before this patch was definitely wrong so I
>>>> consider it an improvement.
>>>>
>>> This is how I feel about it too. Erroring out entirely is an option, but
>>> code-wise just dirtying everything is at least verifiably not-wrong and
>>> pretty simple to implement.
>> But that's exactly the problem: If something is just plain wrong, you
>> can always replace it with something that makes sense. If it errors out,
>> you can still remove that error later. But if you have something that
>> doesn't make a whole lot of sense, but kinda sorta works (like after
>> this patch), it's ABI and you can't implement something more useful
>> later any more.
>>
>>> It's an improvement... Don't do it, but at least you won't get
>>> something wrong after, just something heinously unoptimal.
>> It's a short-term improvement that may become a long-term burden.
>>
>> Kevin
>>
> I see your point.
>
> If we enable it now, we always have to.
> If we disable it now, we *can* later if we wish.
>
> ...however, I think it's been the case that we haven't prohibited it
> before, but also it's a pretty good case that nobody has been using this
> feature in production because they're not yet persistent and migratable.
>
> An error message asking the user to delete bitmaps (which will very
> obviously invalidate them) would be fine, too. I was erring on the side
> of "just let things work," but you have a point that making sure the
> user knows that what they're trying to accomplish is not a good idea is
> probably better than silently doing the very stupid thing.

Hi all. erroring  is ok for me too. And looks like it's a way which 
looks ok for all interested in. Should I make a patch?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Vladimir Sementsov-Ogievskiy]

20.11.2017 12:51, Vladimir Sementsov-Ogievskiy wrote:
> 17.11.2017 23:40, John Snow wrote:
>>
>> On 11/17/2017 01:25 PM, Kevin Wolf wrote:
>>> Am 17.11.2017 um 19:15 hat John Snow geschrieben:
>>>>
>>>> On 11/17/2017 10:01 AM, Max Reitz wrote:
>>>>> On 2017-11-17 13:30, Kevin Wolf wrote:
>>>>>> Am 23.10.2017 um 11:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>>> Snapshot-switch actually changes active state of disk so it should
>>>>>>> reflect on dirty bitmaps. Otherwise next incremental backup using
>>>>>>> these bitmaps will be invalid.
>>>>>>>
>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>>>>>> <vsementsov@virtuozzo.com>
>>>>>> We discussed this quite a while ago, and I'm still not convinced 
>>>>>> that
>>>>>> this approach makes sense.
>>>>> I think it at least makes more sense than not handling this case 
>>>>> at all.
>>>>>
>>>>>> Can you give just one example of a use case where dirtying the whole
>>>>>> bitmap while loading a snapshot is the desired behaviour?
>>>>>>
>>>>>> I think the most useful behaviour would be something where the 
>>>>>> bitmaps
>>>>>> themselves are snapshotted, too.
>>>>> Agreed.
>>>>>
>>>>>> But for the time being, the easiest and
>>>>>> safest solution might just be to error out in any snapshot 
>>>>>> operations
>>>>>> if any bitmaps are in use.
>>>>> Sounds OK, too.  I personally don't have an opinion either way.
>>>>>
>>>>> But in any case, what we did before this patch was definitely 
>>>>> wrong so I
>>>>> consider it an improvement.
>>>>>
>>>> This is how I feel about it too. Erroring out entirely is an 
>>>> option, but
>>>> code-wise just dirtying everything is at least verifiably not-wrong 
>>>> and
>>>> pretty simple to implement.
>>> But that's exactly the problem: If something is just plain wrong, you
>>> can always replace it with something that makes sense. If it errors 
>>> out,
>>> you can still remove that error later. But if you have something that
>>> doesn't make a whole lot of sense, but kinda sorta works (like after
>>> this patch), it's ABI and you can't implement something more useful
>>> later any more.
>>>
>>>> It's an improvement... Don't do it, but at least you won't get
>>>> something wrong after, just something heinously unoptimal.
>>> It's a short-term improvement that may become a long-term burden.
>>>
>>> Kevin
>>>
>> I see your point.
>>
>> If we enable it now, we always have to.
>> If we disable it now, we *can* later if we wish.
>>
>> ...however, I think it's been the case that we haven't prohibited it
>> before, but also it's a pretty good case that nobody has been using this
>> feature in production because they're not yet persistent and migratable.
>>
>> An error message asking the user to delete bitmaps (which will very
>> obviously invalidate them) would be fine, too. I was erring on the side
>> of "just let things work," but you have a point that making sure the
>> user knows that what they're trying to accomplish is not a good idea is
>> probably better than silently doing the very stupid thing.
>
> Hi all. erroring  is ok for me too. And looks like it's a way which 
> looks ok for all interested in. Should I make a patch?
>

Hmm, it looks not so easy: errp infrastructure is absent here, and 
returning just EINVAL is not very informative.

-- 
Best regards,
Vladimir