| Message ID | 20180613020613.1343-4-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <jsnow@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.. 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); > - bitmap_list_free(bm_list); > > return ret; > } > @@ -1265,7 +1287,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > return; > } > > - bm_list = bitmap_list_load(bs, errp); > + bm_list = get_bitmap_list(bs, errp); > if (bm_list == NULL) { > return; > } > @@ -1287,7 +1309,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > > fail: > bitmap_free(bm); > - bitmap_list_free(bm_list); > +} > + > +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs) > +{ > + del_bitmap_list(bs); > } > > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > @@ -1304,23 +1330,19 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > > if (!bdrv_has_changed_persistent_bitmaps(bs)) { > /* nothing to do */ > - return; > + goto out; > } > > if (!can_write(bs)) { > error_setg(errp, "No write access"); > - return; > + goto out; > } > > QSIMPLEQ_INIT(&drop_tables); > > - if (s->nb_bitmaps == 0) { > - bm_list = bitmap_list_new(); > - } else { > - bm_list = bitmap_list_load(bs, errp); > - if (bm_list == NULL) { > - return; > - } > + bm_list = get_bitmap_list(bs, errp); > + if (bm_list == NULL) { > + goto out; > } > > /* check constraints and names */ > @@ -1400,8 +1422,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > g_free(tb); > } > > - bitmap_list_free(bm_list); > - return; > + goto out; > > fail: > QSIMPLEQ_FOREACH(bm, bm_list, entry) { > @@ -1416,7 +1437,9 @@ fail: > g_free(tb); > } > > - bitmap_list_free(bm_list); > + out: > + del_bitmap_list(bs); > + return; > } > > int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) > @@ -1481,13 +1504,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > goto fail; > } > > - bm_list = bitmap_list_load(bs, errp); > + bm_list = get_bitmap_list(bs, errp); > if (bm_list == NULL) { > goto fail; > } > > found = find_bitmap_by_name(bm_list, name); > - bitmap_list_free(bm_list); > if (found) { > error_setg(errp, "Bitmap with the same name is already stored"); > goto fail; > diff --git a/block/qcow2.c b/block/qcow2.c > index 6fa5e1d71a..dbd334b150 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2148,6 +2148,8 @@ static void qcow2_close(BlockDriverState *bs) > > if (!(s->flags & BDRV_O_INACTIVE)) { > qcow2_inactivate(bs); > + } else { > + qcow2_persistent_dirty_bitmaps_cache_destroy(bs); > } > > cache_clean_timer_del(bs); > diff --git a/block/qcow2.h b/block/qcow2.h > index 01b5250415..29b637a0ee 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -295,6 +295,7 @@ typedef struct BDRVQcow2State { > uint64_t bitmap_directory_size; > uint64_t bitmap_directory_offset; > bool dirty_bitmaps_loaded; > + void *bitmap_list; > > int flags; > int qcow_version; > @@ -671,6 +672,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); > int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, > Error **errp); > int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); > +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs); > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); > int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); > bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
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<jsnow@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.. 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 = 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); >> - bitmap_list_free(bm_list); >> >> return ret; >> } >> @@ -1265,7 +1287,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> return; >> } >> >> - bm_list = bitmap_list_load(bs, errp); >> + bm_list = get_bitmap_list(bs, errp); >> if (bm_list == NULL) { >> return; >> } >> @@ -1287,7 +1309,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> >> fail: >> bitmap_free(bm); >> - bitmap_list_free(bm_list); >> +} >> + >> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs) >> +{ >> + del_bitmap_list(bs); >> } >> >> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) >> @@ -1304,23 +1330,19 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) >> >> if (!bdrv_has_changed_persistent_bitmaps(bs)) { >> /* nothing to do */ >> - return; >> + goto out; >> } >> >> if (!can_write(bs)) { >> error_setg(errp, "No write access"); >> - return; >> + goto out; >> } >> >> QSIMPLEQ_INIT(&drop_tables); >> >> - if (s->nb_bitmaps == 0) { >> - bm_list = bitmap_list_new(); >> - } else { >> - bm_list = bitmap_list_load(bs, errp); >> - if (bm_list == NULL) { >> - return; >> - } >> + bm_list = get_bitmap_list(bs, errp); >> + if (bm_list == NULL) { >> + goto out; >> } >> >> /* check constraints and names */ >> @@ -1400,8 +1422,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) >> g_free(tb); >> } >> >> - bitmap_list_free(bm_list); >> - return; >> + goto out; >> >> fail: >> QSIMPLEQ_FOREACH(bm, bm_list, entry) { >> @@ -1416,7 +1437,9 @@ fail: >> g_free(tb); >> } >> >> - bitmap_list_free(bm_list); >> + out: >> + del_bitmap_list(bs); >> + return; >> } >> >> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) >> @@ -1481,13 +1504,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >> goto fail; >> } >> >> - bm_list = bitmap_list_load(bs, errp); >> + bm_list = get_bitmap_list(bs, errp); >> if (bm_list == NULL) { >> goto fail; >> } >> >> found = find_bitmap_by_name(bm_list, name); >> - bitmap_list_free(bm_list); >> if (found) { >> error_setg(errp, "Bitmap with the same name is already stored"); >> goto fail; >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 6fa5e1d71a..dbd334b150 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2148,6 +2148,8 @@ static void qcow2_close(BlockDriverState *bs) >> >> if (!(s->flags & BDRV_O_INACTIVE)) { >> qcow2_inactivate(bs); >> + } else { >> + qcow2_persistent_dirty_bitmaps_cache_destroy(bs); >> } >> >> cache_clean_timer_del(bs); >> diff --git a/block/qcow2.h b/block/qcow2.h >> index 01b5250415..29b637a0ee 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -295,6 +295,7 @@ typedef struct BDRVQcow2State { >> uint64_t bitmap_directory_size; >> uint64_t bitmap_directory_offset; >> bool dirty_bitmaps_loaded; >> + void *bitmap_list; >> >> int flags; >> int qcow_version; >> @@ -671,6 +672,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); >> int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, >> Error **errp); >> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs); >> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >> bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > > > -- > Best regards, > Vladimir
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 <jsnow@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? > 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 ...
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 <jsnow@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 ...
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.)
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; + 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); - bitmap_list_free(bm_list); return ret; } @@ -1265,7 +1287,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, return; } - bm_list = bitmap_list_load(bs, errp); + bm_list = get_bitmap_list(bs, errp); if (bm_list == NULL) { return; } @@ -1287,7 +1309,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, fail: bitmap_free(bm); - bitmap_list_free(bm_list); +} + +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs) +{ + del_bitmap_list(bs); } void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) @@ -1304,23 +1330,19 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) if (!bdrv_has_changed_persistent_bitmaps(bs)) { /* nothing to do */ - return; + goto out; } if (!can_write(bs)) { error_setg(errp, "No write access"); - return; + goto out; } QSIMPLEQ_INIT(&drop_tables); - if (s->nb_bitmaps == 0) { - bm_list = bitmap_list_new(); - } else { - bm_list = bitmap_list_load(bs, errp); - if (bm_list == NULL) { - return; - } + bm_list = get_bitmap_list(bs, errp); + if (bm_list == NULL) { + goto out; } /* check constraints and names */ @@ -1400,8 +1422,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) g_free(tb); } - bitmap_list_free(bm_list); - return; + goto out; fail: QSIMPLEQ_FOREACH(bm, bm_list, entry) { @@ -1416,7 +1437,9 @@ fail: g_free(tb); } - bitmap_list_free(bm_list); + out: + del_bitmap_list(bs); + return; } int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) @@ -1481,13 +1504,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, goto fail; } - bm_list = bitmap_list_load(bs, errp); + bm_list = get_bitmap_list(bs, errp); if (bm_list == NULL) { goto fail; } found = find_bitmap_by_name(bm_list, name); - bitmap_list_free(bm_list); if (found) { error_setg(errp, "Bitmap with the same name is already stored"); goto fail; diff --git a/block/qcow2.c b/block/qcow2.c index 6fa5e1d71a..dbd334b150 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2148,6 +2148,8 @@ static void qcow2_close(BlockDriverState *bs) if (!(s->flags & BDRV_O_INACTIVE)) { qcow2_inactivate(bs); + } else { + qcow2_persistent_dirty_bitmaps_cache_destroy(bs); } cache_clean_timer_del(bs); diff --git a/block/qcow2.h b/block/qcow2.h index 01b5250415..29b637a0ee 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -295,6 +295,7 @@ typedef struct BDRVQcow2State { uint64_t bitmap_directory_size; uint64_t bitmap_directory_offset; bool dirty_bitmaps_loaded; + void *bitmap_list; int flags; int qcow_version; @@ -671,6 +672,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs); void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
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 <jsnow@redhat.com> --- block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++++------------------ block/qcow2.c | 2 ++ block/qcow2.h | 2 ++ 3 files changed, 52 insertions(+), 26 deletions(-)