Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list
On 06/21/2018 06:25 AM, Vladimir Sementsov-Ogievskiy wrote: >> > > agree. and this is one more reason to not load bitmaps in inactive mode > at all. and drop them (after storing) on inactivating. > I'll make a patch. Sure. I guess persistent bitmaps that exist when BDRV_O_INACTIVE is set need to stay around -- it would be strange if they disappeared just because the bitmap is inactive -- but we need to effectively overwrite them on reload from disk. And while the disk is inactive, these bitmaps definitely need to remain in an enforced readonly state (can't be deleted, renamed, cleared, set, reset, etc.) (I guess it would also be an error to try to remove persistence from a bitmap on an inactive disk too.)
Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list
21.06.2018 02:29, John Snow wrote:
On 06/20/2018 09:04 AM, Vladimir Sementsov-Ogievskiy wrote:
13.06.2018 05:06, John Snow wrote:
We don't need to re-read this list every time, exactly. We can keep it
cached
and delete our copy when we flush to disk.
Because we don't try to flush bitmaps on close if there's nothing to
flush,
add a new conditional to delete the state anyway for a clean exit.
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 74
++--
block/qcow2.c | 2 ++
block/qcow2.h | 2 ++
3 files changed, 52 insertions(+), 26 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 85c1b5afe3..5ae9b17928 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -636,6 +636,34 @@ fail:
return NULL;
}
+static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error
**errp)
+{
+ BDRVQcow2State *s = bs->opaque;
+ Qcow2BitmapList *bm_list;
+
+ if (s->bitmap_list) {
+ return (Qcow2BitmapList *)s->bitmap_list;
+ }
+
+ if (s->nb_bitmaps) {
+ bm_list = bitmap_list_load(bs, errp);
+ } else {
+ bm_list = bitmap_list_new();
+ }
+ s->bitmap_list = bm_list;
may be, we shouldn't cache it in inactive mode. However, I think we'll
finally will not load bitmaps in inactive mode and drop the on
inactivate, so it would not matter..
Do we not load bitmaps when BDRV_O_INACTIVE is set? it looks like we do?
(From your subsequent email):
really, now it would be a problem: we can start in inactive mode, load
nothing, and then we can't reload bitmaps; my fix in
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03254.html
will not work after this patch.
So we load nothing because when we opened up the image RO, saw IN_USE
bitmaps (or saw none at all) and decided not to load them. qcow2_do_open
however marks that it has "loaded the bitmaps."
Later, when we reopen RW, we have a cached bm_list that doesn't include
this bitmap, so we don't mark it as IN_USE again or update the header.
(Wait, if you are worried about the bitmap's data having been changed,
why do we not reload the bitmap data here too?)
Now, patch 06 changes the cache so that we load all bitmaps and not just
ones not IN_USE. On RW reload, I reject any such IN_USE bitmaps as a
reason to prohibit the RW reload. However, this is broken too, because I
will miss any new flags that exist on-disk, so this function should
never use the cached data.
I'm confused, though, the comment that calls reopen_bitmaps_rw_hint says:
"It's some kind of reopen. There are no known cases where we need to
reload bitmaps in such a situation, so it's safer to skip them.
Moreover, if we have some readonly bitmaps and we are reopening for rw
we should reopen bitmaps correspondingly."
Why do we assume that bitmap data cannot change while BDRV_O_INACTIVATE
is set? Is that not wrong?
agree. and this is one more reason to not load bitmaps in inactive mode
at all. and drop them (after storing) on inactivating.
I'll make a patch.
Hm, I've understood the following problem: cache becomes incorrect after
failed update_ext_header_and_dir or update_ext_header_and_dir_in_place
operations. (after failed qcow2_remove_persistent_dirty_bitmap or
qcow2_reopen_bitmaps_rw_hint)
And this comes from incomplete architecture after the patch:
On the one hand, we work with one singleton bitmap_list, and loading
part is refactored to do it safely. On the other hand, storing functions
still have old behavior, they work with bitmap list like with their own
local variable.
Yeah, I see it. Dropping the cache on such errors is fine.
So, we have safe mechanism to read list through the cache. We need also
safe mechanism to update list both in cache and in file.
There are two possible variants:
1. drop cache after failed store
2. rollback cache after failed store
1 looks simpler..
Also, we should drop cache on inactivate (we do this) and should not
create cache in inactive mode, because the other process may change the
image.
Hm. may be, it is better to work with s->bitmap_list directly? In this
case it will be more obvious that it is the cache, not local variable.
And we will work with it like with other "parts of extension cache"
s->nb_bitmaps, s->bitmap_directory_offset ...
After the patch, functions update_ext_header_and_dir* becomes strange:
1. before the patch, they take external parameter - bm_list, and by this
parameter they updated the file and cached s->nb_bitmaps, s->bitmap_*, ..
2. after the patch, they take parameter (actually s->bitmap_list) of
same nature like s->nb_bitmap, and update s->nb_bitmap from it.
Yeah, if we do decide that keeping a cache is the right thing, some of
the helper functions could be refactored or simplified a little to take
advantage of the new paradigm.
Sorry for being late and for disordered stream of thoughts. Is this
patch really needed for the whole series?
The nice part is to
Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list
On 06/20/2018 09:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2018 05:06, John Snow wrote:
>> We don't need to re-read this list every time, exactly. We can keep it
>> cached
>> and delete our copy when we flush to disk.
>>
>> Because we don't try to flush bitmaps on close if there's nothing to
>> flush,
>> add a new conditional to delete the state anyway for a clean exit.
>>
>> Signed-off-by: John Snow
>> ---
>> block/qcow2-bitmap.c | 74
>> ++--
>> block/qcow2.c | 2 ++
>> block/qcow2.h | 2 ++
>> 3 files changed, 52 insertions(+), 26 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 85c1b5afe3..5ae9b17928 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -636,6 +636,34 @@ fail:
>> return NULL;
>> }
>> +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error
>> **errp)
>> +{
>> + BDRVQcow2State *s = bs->opaque;
>> + Qcow2BitmapList *bm_list;
>> +
>> + if (s->bitmap_list) {
>> + return (Qcow2BitmapList *)s->bitmap_list;
>> + }
>> +
>> + if (s->nb_bitmaps) {
>> + bm_list = bitmap_list_load(bs, errp);
>> + } else {
>> + bm_list = bitmap_list_new();
>> + }
>> + s->bitmap_list = bm_list;
>
> may be, we shouldn't cache it in inactive mode. However, I think we'll
> finally will not load bitmaps in inactive mode and drop the on
> inactivate, so it would not matter..
>
Do we not load bitmaps when BDRV_O_INACTIVE is set? it looks like we do?
(From your subsequent email):
>
> really, now it would be a problem: we can start in inactive mode, load
> nothing, and then we can't reload bitmaps; my fix in
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03254.html
> will not work after this patch.
>
So we load nothing because when we opened up the image RO, saw IN_USE
bitmaps (or saw none at all) and decided not to load them. qcow2_do_open
however marks that it has "loaded the bitmaps."
Later, when we reopen RW, we have a cached bm_list that doesn't include
this bitmap, so we don't mark it as IN_USE again or update the header.
(Wait, if you are worried about the bitmap's data having been changed,
why do we not reload the bitmap data here too?)
Now, patch 06 changes the cache so that we load all bitmaps and not just
ones not IN_USE. On RW reload, I reject any such IN_USE bitmaps as a
reason to prohibit the RW reload. However, this is broken too, because I
will miss any new flags that exist on-disk, so this function should
never use the cached data.
I'm confused, though, the comment that calls reopen_bitmaps_rw_hint says:
"It's some kind of reopen. There are no known cases where we need to
reload bitmaps in such a situation, so it's safer to skip them.
Moreover, if we have some readonly bitmaps and we are reopening for rw
we should reopen bitmaps correspondingly."
Why do we assume that bitmap data cannot change while BDRV_O_INACTIVATE
is set? Is that not wrong?
> Hm, I've understood the following problem: cache becomes incorrect after
> failed update_ext_header_and_dir or update_ext_header_and_dir_in_place
> operations. (after failed qcow2_remove_persistent_dirty_bitmap or
> qcow2_reopen_bitmaps_rw_hint)
>
> And this comes from incomplete architecture after the patch:
> On the one hand, we work with one singleton bitmap_list, and loading
> part is refactored to do it safely. On the other hand, storing functions
> still have old behavior, they work with bitmap list like with their own
> local variable.
Yeah, I see it. Dropping the cache on such errors is fine.
>
> So, we have safe mechanism to read list through the cache. We need also
> safe mechanism to update list both in cache and in file.
>
> There are two possible variants:
>
> 1. drop cache after failed store
> 2. rollback cache after failed store
>
> 1 looks simpler..
>
> Also, we should drop cache on inactivate (we do this) and should not
> create cache in inactive mode, because the other process may change the
> image.
>
> Hm. may be, it is better to work with s->bitmap_list directly? In this
> case it will be more obvious that it is the cache, not local variable.
> And we will work with it like with other "parts of extension cache"
> s->nb_bitmaps, s->bitmap_directory_offset ...
>
> After the patch, functions update_ext_header_and_dir* becomes strange:
>
> 1. before the patch, they take external parameter - bm_list, and by this
> parameter they updated the file and cached s->nb_bitmaps, s->bitmap_*, ..
> 2. after the patch, they take parameter (actually s->bitmap_list) of
> same nature like s->nb_bitmap, and update s->nb_bitmap from it.
>
Yeah, if we do decide that keeping a cache is the right thing, some of
the helper functions could be refactored or simplified a little to take
advantage of the new paradigm.
> Sorry for being late and for disordered stream of thoughts. Is this
> patch really needed for the whole series?
>
Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list
20.06.2018 16:04, Vladimir Sementsov-Ogievskiy wrote:
13.06.2018 05:06, John Snow wrote:
We don't need to re-read this list every time, exactly. We can keep it cached
and delete our copy when we flush to disk.
Because we don't try to flush bitmaps on close if there's nothing to flush,
add a new conditional to delete the state anyway for a clean exit.
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 74 ++--
block/qcow2.c| 2 ++
block/qcow2.h| 2 ++
3 files changed, 52 insertions(+), 26 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 85c1b5afe3..5ae9b17928 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -636,6 +636,34 @@ fail:
return NULL;
}
+static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+Qcow2BitmapList *bm_list;
+
+if (s->bitmap_list) {
+return (Qcow2BitmapList *)s->bitmap_list;
+}
+
+if (s->nb_bitmaps) {
+bm_list = bitmap_list_load(bs, errp);
+} else {
+bm_list = bitmap_list_new();
+}
+s->bitmap_list = bm_list;
may be, we shouldn't cache it in inactive mode. However, I think we'll
finally will not load bitmaps in inactive mode and drop the on
inactivate, so it would not matter..
really, now it would be a problem: we can start in inactive mode, load
nothing, and then we can't reload bitmaps; my fix in
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03254.html
will not work after this patch.
Hm, I've understood the following problem: cache becomes incorrect
after failed update_ext_header_and_dir or
update_ext_header_and_dir_in_place operations. (after failed
qcow2_remove_persistent_dirty_bitmap or qcow2_reopen_bitmaps_rw_hint)
And this comes from incomplete architecture after the patch:
On the one hand, we work with one singleton bitmap_list, and loading
part is refactored to do it safely. On the other hand, storing
functions still have old behavior, they work with bitmap list like
with their own local variable.
So, we have safe mechanism to read list through the cache. We need
also safe mechanism to update list both in cache and in file.
There are two possible variants:
1. drop cache after failed store
2. rollback cache after failed store
1 looks simpler..
Also, we should drop cache on inactivate (we do this) and should not
create cache in inactive mode, because the other process may change
the image.
Hm. may be, it is better to work with s->bitmap_list directly? In this
case it will be more obvious that it is the cache, not local variable.
And we will work with it like with other "parts of extension cache"
s->nb_bitmaps, s->bitmap_directory_offset ...
After the patch, functions update_ext_header_and_dir* becomes strange:
1. before the patch, they take external parameter - bm_list, and by
this parameter they updated the file and cached s->nb_bitmaps,
s->bitmap_*, ..
2. after the patch, they take parameter (actually s->bitmap_list) of
same nature like s->nb_bitmap, and update s->nb_bitmap from it.
Sorry for being late and for disordered stream of thoughts. Is this
patch really needed for the whole series?
+return bm_list;
+}
+
+static void del_bitmap_list(BlockDriverState *bs)
+{
+BDRVQcow2State *s = bs->opaque;
+
+if (s->bitmap_list) {
+bitmap_list_free(s->bitmap_list);
+s->bitmap_list = NULL;
+}
+}
+
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size)
@@ -656,7 +684,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
return ret;
}
-bm_list = bitmap_list_load(bs, NULL);
+bm_list = get_bitmap_list(bs, NULL);
if (bm_list == NULL) {
res->corruptions++;
return -EINVAL;
@@ -706,8 +734,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
}
out:
-bitmap_list_free(bm_list);
-
return ret;
}
@@ -944,7 +970,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
return false;
}
-bm_list = bitmap_list_load(bs, errp);
+bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
return false;
}
@@ -978,8 +1004,6 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
}
}
-bitmap_list_free(bm_list);
-
return needs_update;
fail:
@@ -988,8 +1012,7 @@ fail:
bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
}
}
-bitmap_list_free(bm_list);
-
+del_bitmap_list(bs);
return false;
}
@@ -1016,7 +1039,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
return -EINVAL;
}
-bm_list =
Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list
13.06.2018 05:06, John Snow wrote:
We don't need to re-read this list every time, exactly. We can keep it cached
and delete our copy when we flush to disk.
Because we don't try to flush bitmaps on close if there's nothing to flush,
add a new conditional to delete the state anyway for a clean exit.
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 74 ++--
block/qcow2.c| 2 ++
block/qcow2.h| 2 ++
3 files changed, 52 insertions(+), 26 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 85c1b5afe3..5ae9b17928 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -636,6 +636,34 @@ fail:
return NULL;
}
+static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+Qcow2BitmapList *bm_list;
+
+if (s->bitmap_list) {
+return (Qcow2BitmapList *)s->bitmap_list;
+}
+
+if (s->nb_bitmaps) {
+bm_list = bitmap_list_load(bs, errp);
+} else {
+bm_list = bitmap_list_new();
+}
+s->bitmap_list = bm_list;
may be, we shouldn't cache it in inactive mode. However, I think we'll
finally will not load bitmaps in inactive mode and drop the on
inactivate, so it would not matter..
Hm, I've understood the following problem: cache becomes incorrect after
failed update_ext_header_and_dir or update_ext_header_and_dir_in_place
operations. (after failed qcow2_remove_persistent_dirty_bitmap or
qcow2_reopen_bitmaps_rw_hint)
And this comes from incomplete architecture after the patch:
On the one hand, we work with one singleton bitmap_list, and loading
part is refactored to do it safely. On the other hand, storing functions
still have old behavior, they work with bitmap list like with their own
local variable.
So, we have safe mechanism to read list through the cache. We need also
safe mechanism to update list both in cache and in file.
There are two possible variants:
1. drop cache after failed store
2. rollback cache after failed store
1 looks simpler..
Also, we should drop cache on inactivate (we do this) and should not
create cache in inactive mode, because the other process may change the
image.
Hm. may be, it is better to work with s->bitmap_list directly? In this
case it will be more obvious that it is the cache, not local variable.
And we will work with it like with other "parts of extension cache"
s->nb_bitmaps, s->bitmap_directory_offset ...
After the patch, functions update_ext_header_and_dir* becomes strange:
1. before the patch, they take external parameter - bm_list, and by this
parameter they updated the file and cached s->nb_bitmaps, s->bitmap_*, ..
2. after the patch, they take parameter (actually s->bitmap_list) of
same nature like s->nb_bitmap, and update s->nb_bitmap from it.
Sorry for being late and for disordered stream of thoughts. Is this
patch really needed for the whole series?
+return bm_list;
+}
+
+static void del_bitmap_list(BlockDriverState *bs)
+{
+BDRVQcow2State *s = bs->opaque;
+
+if (s->bitmap_list) {
+bitmap_list_free(s->bitmap_list);
+s->bitmap_list = NULL;
+}
+}
+
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size)
@@ -656,7 +684,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
return ret;
}
-bm_list = bitmap_list_load(bs, NULL);
+bm_list = get_bitmap_list(bs, NULL);
if (bm_list == NULL) {
res->corruptions++;
return -EINVAL;
@@ -706,8 +734,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
}
out:
-bitmap_list_free(bm_list);
-
return ret;
}
@@ -944,7 +970,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
return false;
}
-bm_list = bitmap_list_load(bs, errp);
+bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
return false;
}
@@ -978,8 +1004,6 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
}
}
-bitmap_list_free(bm_list);
-
return needs_update;
fail:
@@ -988,8 +1012,7 @@ fail:
bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
}
}
-bitmap_list_free(bm_list);
-
+del_bitmap_list(bs);
return false;
}
@@ -1016,7 +1039,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
return -EINVAL;
}
-bm_list = bitmap_list_load(bs, errp);
+bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
return -EINVAL;
}
@@ -1056,7 +1079,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs,
bool *header_updated,
out:
g_slist_free(ro_dirty_bitmaps);
-
