| Message ID | 20180411122606.367301-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps persistent and migration fixes | expand |
On 04/11/2018 07:26 AM, Vladimir Sementsov-Ogievskiy wrote: > Checking reopen by existence of some bitmaps is wrong, as it may be > some other bitmaps, or on the other hand, user may remove bitmaps. This > criteria is bad. To simplify things and make behavior more predictable > let's just add a flag to remember, that we've already tried to load > bitmaps on open and do not want do it again. Wording suggestion: Checking whether we are reopening an image based on the existence of some bitmaps is wrong, as the set of bitmaps may have changed (for example, the user may have added or removed bitmaps in the meantime). To simplify things and make behavior more predictable, let's just add a flag to remember whether we've already tried to load bitmaps, so that we only attempt it on the initial open. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.h | 1 + > block/qcow2.c | 16 ++++++++-------- > 2 files changed, 9 insertions(+), 8 deletions(-) > > @@ -1480,10 +1481,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, > s->autoclear_features &= QCOW2_AUTOCLEAR_MASK; > } > > - if (bdrv_dirty_bitmap_next(bs, NULL)) { > - /* It's some kind of reopen with already existing dirty bitmaps. There > - * are no known cases where we need loading bitmaps in such situation, > - * so it's safer don't load them. > + if (s->dirty_bitmaps_loaded) { > + /* It's some kind of reopen. There are no known cases where we need > + * loading bitmaps in such situation, so it's safer don't load them. Pre-existing wording, but sounds better as: 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. > @@ -1491,13 +1491,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, > if (bdrv_has_readonly_bitmaps(bs) && > !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) > { > - bool header_updated = false; > qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err); > - update_header = update_header && !header_updated; > } > - } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) { > - update_header = false; > + } else { > + header_updated = qcow2_load_dirty_bitmaps(bs, &local_err); > + s->dirty_bitmaps_loaded = true; > } > + update_header = update_header && !header_updated; Could write this as 'update_header &= !header_updated;'
11.04.2018 16:22, Eric Blake wrote: > On 04/11/2018 07:26 AM, Vladimir Sementsov-Ogievskiy wrote: >> Checking reopen by existence of some bitmaps is wrong, as it may be >> some other bitmaps, or on the other hand, user may remove bitmaps. This >> criteria is bad. To simplify things and make behavior more predictable >> let's just add a flag to remember, that we've already tried to load >> bitmaps on open and do not want do it again. > Wording suggestion: > > Checking whether we are reopening an image based on the existence of > some bitmaps is wrong, as the set of bitmaps may have changed (for > example, the user may have added or removed bitmaps in the meantime). > To simplify things and make behavior more predictable, let's just add a > flag to remember whether we've already tried to load bitmaps, so that we > only attempt it on the initial open. You've dropped an idea supposed by Max, about migration related bitmaps, I said "some other bitmaps", because now I doubt is it possible: we have migration related bitmaps on source, not on target.. Anyway, I'm ok with all your suggestions, thank you. I can respin, or they may be squashed-in inflight. > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2.h | 1 + >> block/qcow2.c | 16 ++++++++-------- >> 2 files changed, 9 insertions(+), 8 deletions(-) >> >> @@ -1480,10 +1481,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, >> s->autoclear_features &= QCOW2_AUTOCLEAR_MASK; >> } >> >> - if (bdrv_dirty_bitmap_next(bs, NULL)) { >> - /* It's some kind of reopen with already existing dirty bitmaps. There >> - * are no known cases where we need loading bitmaps in such situation, >> - * so it's safer don't load them. >> + if (s->dirty_bitmaps_loaded) { >> + /* It's some kind of reopen. There are no known cases where we need >> + * loading bitmaps in such situation, so it's safer don't load them. > Pre-existing wording, but sounds better as: > > 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. >> @@ -1491,13 +1491,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, >> if (bdrv_has_readonly_bitmaps(bs) && >> !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) >> { >> - bool header_updated = false; >> qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err); >> - update_header = update_header && !header_updated; >> } >> - } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) { >> - update_header = false; >> + } else { >> + header_updated = qcow2_load_dirty_bitmaps(bs, &local_err); >> + s->dirty_bitmaps_loaded = true; >> } >> + update_header = update_header && !header_updated; > Could write this as 'update_header &= !header_updated;' >
On 2018-04-11 14:26, Vladimir Sementsov-Ogievskiy wrote: > Checking reopen by existence of some bitmaps is wrong, as it may be > some other bitmaps, or on the other hand, user may remove bitmaps. This > criteria is bad. To simplify things and make behavior more predictable > let's just add a flag to remember, that we've already tried to load > bitmaps on open and do not want do it again. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.h | 1 + > block/qcow2.c | 16 ++++++++-------- > 2 files changed, 9 insertions(+), 8 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
diff --git a/block/qcow2.h b/block/qcow2.h index d301f77cea..adf5c3950f 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -298,6 +298,7 @@ typedef struct BDRVQcow2State { uint32_t nb_bitmaps; uint64_t bitmap_directory_size; uint64_t bitmap_directory_offset; + bool dirty_bitmaps_loaded; int flags; int qcow_version; diff --git a/block/qcow2.c b/block/qcow2.c index 486f3e83b7..4242e99f1e 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1142,6 +1142,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, uint64_t ext_end; uint64_t l1_vm_state_index; bool update_header = false; + bool header_updated = false; ret = bdrv_pread(bs->file, 0, &header, sizeof(header)); if (ret < 0) { @@ -1480,10 +1481,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, s->autoclear_features &= QCOW2_AUTOCLEAR_MASK; } - if (bdrv_dirty_bitmap_next(bs, NULL)) { - /* It's some kind of reopen with already existing dirty bitmaps. There - * are no known cases where we need loading bitmaps in such situation, - * so it's safer don't load them. + if (s->dirty_bitmaps_loaded) { + /* It's some kind of reopen. There are no known cases where we need + * loading bitmaps in such situation, so it's safer don't load them. * * Moreover, if we have some readonly bitmaps and we are reopening for * rw we should reopen bitmaps correspondingly. @@ -1491,13 +1491,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, if (bdrv_has_readonly_bitmaps(bs) && !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) { - bool header_updated = false; qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err); - update_header = update_header && !header_updated; } - } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) { - update_header = false; + } else { + header_updated = qcow2_load_dirty_bitmaps(bs, &local_err); + s->dirty_bitmaps_loaded = true; } + update_header = update_header && !header_updated; if (local_err != NULL) { error_propagate(errp, local_err); ret = -EINVAL;
Checking reopen by existence of some bitmaps is wrong, as it may be some other bitmaps, or on the other hand, user may remove bitmaps. This criteria is bad. To simplify things and make behavior more predictable let's just add a flag to remember, that we've already tried to load bitmaps on open and do not want do it again. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2.h | 1 + block/qcow2.c | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-)