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 <js...@redhat.com>
---
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 have bm_list with pointers that correlate directly
to the in-memory bitmaps.
I can load bm_list on demand later for the purposes of `qemu-img info`,
but I have to look up the in-memory bitmaps again too. It seemed simpler
to just preserve the data the first time we build it instead of
rebuilding it all the time.
I think that we'll eventually want a cache either way, but maybe I am
still underestimating how difficult it is to do correctly... I think I
need to understand inactivate/invalidate a little more carefully before
I trudge ahead here ...
--
Best regards,
Vladimir
