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
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
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);
> }
>
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
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
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
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
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
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
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 >
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
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.
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
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
© 2016 - 2024 Red Hat, Inc.