| Message ID | 20190305234337.18353-5-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/qcow2-bitmap: Enable resize with persistent bitmaps | expand |
06.03.2019 2:43, John Snow wrote: > Since we now load all bitmaps into memory anyway, we can just truncate them > in-memory and then flush them back to disk. Just in case, we will still check > and enforce that this shortcut is valid -- i.e. that any bitmap described > on-disk is indeed in-memory and can be modified. > > If there are any inconsistent bitmaps, we refuse to allow the truncate as > we do not actually load these bitmaps into memory, and it isn't safe or > reasonable to attempt to truncate corrupted data. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/qcow2.h | 1 + > block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > block/qcow2.c | 27 ++++++++++++++++++++------- > 3 files changed, 63 insertions(+), 7 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index 4c1fdfcbec..b9227a72cc 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, > int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, > Error **errp); > int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); > +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); > void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); > int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 9373055d3b..24f280bccc 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) > return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp); > } > > +/* Checks to see if it's safe to resize bitmaps */ > +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp) > +{ > + BDRVQcow2State *s = bs->opaque; > + Qcow2BitmapList *bm_list; > + Qcow2Bitmap *bm; > + int ret = 0; > + > + if (s->nb_bitmaps == 0) { > + return 0; > + } > + > + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, > + s->bitmap_directory_size, false, errp); > + if (bm_list == NULL) { > + return -EINVAL; > + } > + > + QSIMPLEQ_FOREACH(bm, bm_list, entry) { > + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name); > + if (bitmap == NULL) { > + /* We rely on all bitmaps being in-memory to be able to resize them, > + * Otherwise, we'd need to resize them on disk explicitly */ > + error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that " > + "were not loaded into memory"); > + ret = -ENOTSUP; > + goto out; > + } > + > + /* The checks against readonly and busy are redundant, but certainly > + * do no harm. checks against inconsistent are crucial: */ > + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) { > + ret = -ENOTSUP; > + goto out; > + } > + } > + > +out: > + bitmap_list_free(bm_list); > + return ret; > +} > + > /* store_bitmap_data() > * Store bitmap to image, filling bitmap table accordingly. > */ > diff --git a/block/qcow2.c b/block/qcow2.c > index 7fb2730f09..6fd9252494 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > { > BDRVQcow2State *s = bs->opaque; > uint64_t old_length; > - int64_t new_l1_size; > + int64_t new_l1_size, offset_be; > int ret; > QDict *options; > > @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > goto fail; > } > > - /* cannot proceed if image has bitmaps */ > - if (s->nb_bitmaps) { > - /* TODO: resize bitmaps in the image */ > - error_setg(errp, "Can't resize an image which has bitmaps"); > + /* Only certain persistent bitmaps can be resized: */ > + if (qcow2_truncate_bitmaps_check(bs, errp)) { > ret = -ENOTSUP; > goto fail; > } > @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > bs->total_sectors = offset / BDRV_SECTOR_SIZE; > > /* write updated header.size */ > - offset = cpu_to_be64(offset); > + offset_be = cpu_to_be64(offset); > ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), > - &offset, sizeof(uint64_t)); > + &offset_be, sizeof(uint64_t)); > if (ret < 0) { > error_setg_errno(errp, -ret, "Failed to update the image size"); > goto fail; > @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > if (ret < 0) { > goto fail; > } > + > + /* Flush bitmaps */ > + if (s->nb_bitmaps) { > + Error *local_err = NULL; > + > + /* Usually, bitmaps get resized after this call. > + * Force it earlier so we can make the metadata consistent. */ > + bdrv_dirty_bitmap_truncate(bs, offset); > + qcow2_flush_persistent_dirty_bitmaps(bs, &local_err); > + if (local_err) { > + ret = -EINVAL; > + goto fail; > + } > + } Why to flush after resize? Bitmaps will be IN_USE in the image anyway... Could we implement resize without flush first, it would be one patch + test? And then consider flushing in separate? > + > ret = 0; > fail: > qemu_co_mutex_unlock(&s->lock); >
On 3/6/19 9:33 AM, Vladimir Sementsov-Ogievskiy wrote: >> + /* Flush bitmaps */ >> + if (s->nb_bitmaps) { >> + Error *local_err = NULL; >> + >> + /* Usually, bitmaps get resized after this call. >> + * Force it earlier so we can make the metadata consistent. */ >> + bdrv_dirty_bitmap_truncate(bs, offset); >> + qcow2_flush_persistent_dirty_bitmaps(bs, &local_err); >> + if (local_err) { >> + ret = -EINVAL; >> + goto fail; >> + } >> + } > > Why to flush after resize? Bitmaps will be IN_USE in the image anyway... > > Could we implement resize without flush first, it would be one patch + test? And then consider > flushing in separate? What happens with migration if we don't flush the new size to disk, so the on-disk format has a different size than the in-memory version?
On 3/6/19 10:33 AM, Vladimir Sementsov-Ogievskiy wrote: > 06.03.2019 2:43, John Snow wrote: >> Since we now load all bitmaps into memory anyway, we can just truncate them >> in-memory and then flush them back to disk. Just in case, we will still check >> and enforce that this shortcut is valid -- i.e. that any bitmap described >> on-disk is indeed in-memory and can be modified. >> >> If there are any inconsistent bitmaps, we refuse to allow the truncate as >> we do not actually load these bitmaps into memory, and it isn't safe or >> reasonable to attempt to truncate corrupted data. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/qcow2.h | 1 + >> block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> block/qcow2.c | 27 ++++++++++++++++++++------- >> 3 files changed, 63 insertions(+), 7 deletions(-) >> >> diff --git a/block/qcow2.h b/block/qcow2.h >> index 4c1fdfcbec..b9227a72cc 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, >> int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, >> Error **errp); >> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >> void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >> index 9373055d3b..24f280bccc 100644 >> --- a/block/qcow2-bitmap.c >> +++ b/block/qcow2-bitmap.c >> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) >> return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp); >> } >> >> +/* Checks to see if it's safe to resize bitmaps */ >> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp) >> +{ >> + BDRVQcow2State *s = bs->opaque; >> + Qcow2BitmapList *bm_list; >> + Qcow2Bitmap *bm; >> + int ret = 0; >> + >> + if (s->nb_bitmaps == 0) { >> + return 0; >> + } >> + >> + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, >> + s->bitmap_directory_size, false, errp); >> + if (bm_list == NULL) { >> + return -EINVAL; >> + } >> + >> + QSIMPLEQ_FOREACH(bm, bm_list, entry) { >> + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name); >> + if (bitmap == NULL) { >> + /* We rely on all bitmaps being in-memory to be able to resize them, >> + * Otherwise, we'd need to resize them on disk explicitly */ >> + error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that " >> + "were not loaded into memory"); >> + ret = -ENOTSUP; >> + goto out; >> + } >> + >> + /* The checks against readonly and busy are redundant, but certainly >> + * do no harm. checks against inconsistent are crucial: */ >> + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) { >> + ret = -ENOTSUP; >> + goto out; >> + } >> + } >> + >> +out: >> + bitmap_list_free(bm_list); >> + return ret; >> +} >> + >> /* store_bitmap_data() >> * Store bitmap to image, filling bitmap table accordingly. >> */ >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 7fb2730f09..6fd9252494 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >> { >> BDRVQcow2State *s = bs->opaque; >> uint64_t old_length; >> - int64_t new_l1_size; >> + int64_t new_l1_size, offset_be; >> int ret; >> QDict *options; >> >> @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >> goto fail; >> } >> >> - /* cannot proceed if image has bitmaps */ >> - if (s->nb_bitmaps) { >> - /* TODO: resize bitmaps in the image */ >> - error_setg(errp, "Can't resize an image which has bitmaps"); >> + /* Only certain persistent bitmaps can be resized: */ >> + if (qcow2_truncate_bitmaps_check(bs, errp)) { >> ret = -ENOTSUP; >> goto fail; >> } >> @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >> bs->total_sectors = offset / BDRV_SECTOR_SIZE; >> >> /* write updated header.size */ >> - offset = cpu_to_be64(offset); >> + offset_be = cpu_to_be64(offset); >> ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), >> - &offset, sizeof(uint64_t)); >> + &offset_be, sizeof(uint64_t)); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "Failed to update the image size"); >> goto fail; >> @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >> if (ret < 0) { >> goto fail; >> } >> + >> + /* Flush bitmaps */ >> + if (s->nb_bitmaps) { >> + Error *local_err = NULL; >> + >> + /* Usually, bitmaps get resized after this call. >> + * Force it earlier so we can make the metadata consistent. */ >> + bdrv_dirty_bitmap_truncate(bs, offset); >> + qcow2_flush_persistent_dirty_bitmaps(bs, &local_err); >> + if (local_err) { >> + ret = -EINVAL; >> + goto fail; >> + } >> + } > > Why to flush after resize? Bitmaps will be IN_USE in the image anyway... > > Could we implement resize without flush first, it would be one patch + test? And then consider > flushing in separate? > If you don't flush it to disk, all calls to retrieve the bm_list begin failing because of inconsistent metadata. Future calls to add bitmaps, remove bitmaps, etc will start failing. It seemed safest and easiest to just flush the whole suite to disk. I am trying to avoid premature optimizations at this stage, as I believe resizes will be very infrequent events. --js
06.03.2019 18:36, Eric Blake wrote: > On 3/6/19 9:33 AM, Vladimir Sementsov-Ogievskiy wrote: > >>> + /* Flush bitmaps */ >>> + if (s->nb_bitmaps) { >>> + Error *local_err = NULL; >>> + >>> + /* Usually, bitmaps get resized after this call. >>> + * Force it earlier so we can make the metadata consistent. */ >>> + bdrv_dirty_bitmap_truncate(bs, offset); >>> + qcow2_flush_persistent_dirty_bitmaps(bs, &local_err); >>> + if (local_err) { >>> + ret = -EINVAL; >>> + goto fail; >>> + } >>> + } >> >> Why to flush after resize? Bitmaps will be IN_USE in the image anyway... >> >> Could we implement resize without flush first, it would be one patch + test? And then consider >> flushing in separate? > > What happens with migration if we don't flush the new size to disk, so > the on-disk format has a different size than the in-memory version? > migration works fine, as we store bitmap not on close but on inactivate. Bitmaps may be migrated in two ways: 1. dirty-bitmaps migration capability enabled: they migrate through migration channel, RAM to RAM, BdrvDirtyBitmap to BdrvDirtyBitmap, stored bitmaps are unrelated and they are marked IN_USE anyway 2. dirty-bitmaps migration capability disabled: they migrate through storage, so they are stored in inactivation on source and then loaded in invalidation on target. A good chunk of information is also in a huge comment inside qcow2_do_open() function. Note: we use the following hack for resize, I never tried to send it to the list: in qcow2_co_truncated, instead of "Can't resize an image which has bitmaps": if (s->nb_bitmaps) { /* FIXME: not loaded bitmaps will be lost */ Error *local_err = NULL; qcow2_remove_all_persistent_dirty_bitmaps(bs, &local_err); if (local_err) { error_propagate(errp, local_err); return -EINVAL; } }
06.03.2019 18:41, John Snow wrote: > > > On 3/6/19 10:33 AM, Vladimir Sementsov-Ogievskiy wrote: >> 06.03.2019 2:43, John Snow wrote: >>> Since we now load all bitmaps into memory anyway, we can just truncate them >>> in-memory and then flush them back to disk. Just in case, we will still check >>> and enforce that this shortcut is valid -- i.e. that any bitmap described >>> on-disk is indeed in-memory and can be modified. >>> >>> If there are any inconsistent bitmaps, we refuse to allow the truncate as >>> we do not actually load these bitmaps into memory, and it isn't safe or >>> reasonable to attempt to truncate corrupted data. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> block/qcow2.h | 1 + >>> block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >>> block/qcow2.c | 27 ++++++++++++++++++++------- >>> 3 files changed, 63 insertions(+), 7 deletions(-) >>> >>> diff --git a/block/qcow2.h b/block/qcow2.h >>> index 4c1fdfcbec..b9227a72cc 100644 >>> --- a/block/qcow2.h >>> +++ b/block/qcow2.h >>> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, >>> int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, >>> Error **errp); >>> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >>> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>> void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>> index 9373055d3b..24f280bccc 100644 >>> --- a/block/qcow2-bitmap.c >>> +++ b/block/qcow2-bitmap.c >>> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) >>> return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp); >>> } >>> >>> +/* Checks to see if it's safe to resize bitmaps */ >>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp) >>> +{ >>> + BDRVQcow2State *s = bs->opaque; >>> + Qcow2BitmapList *bm_list; >>> + Qcow2Bitmap *bm; >>> + int ret = 0; >>> + >>> + if (s->nb_bitmaps == 0) { >>> + return 0; >>> + } >>> + >>> + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, >>> + s->bitmap_directory_size, false, errp); >>> + if (bm_list == NULL) { >>> + return -EINVAL; >>> + } >>> + >>> + QSIMPLEQ_FOREACH(bm, bm_list, entry) { >>> + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name); >>> + if (bitmap == NULL) { >>> + /* We rely on all bitmaps being in-memory to be able to resize them, >>> + * Otherwise, we'd need to resize them on disk explicitly */ >>> + error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that " >>> + "were not loaded into memory"); >>> + ret = -ENOTSUP; >>> + goto out; >>> + } >>> + >>> + /* The checks against readonly and busy are redundant, but certainly >>> + * do no harm. checks against inconsistent are crucial: */ >>> + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) { >>> + ret = -ENOTSUP; >>> + goto out; >>> + } >>> + } >>> + >>> +out: >>> + bitmap_list_free(bm_list); >>> + return ret; >>> +} >>> + >>> /* store_bitmap_data() >>> * Store bitmap to image, filling bitmap table accordingly. >>> */ >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index 7fb2730f09..6fd9252494 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>> { >>> BDRVQcow2State *s = bs->opaque; >>> uint64_t old_length; >>> - int64_t new_l1_size; >>> + int64_t new_l1_size, offset_be; >>> int ret; >>> QDict *options; >>> >>> @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>> goto fail; >>> } >>> >>> - /* cannot proceed if image has bitmaps */ >>> - if (s->nb_bitmaps) { >>> - /* TODO: resize bitmaps in the image */ >>> - error_setg(errp, "Can't resize an image which has bitmaps"); >>> + /* Only certain persistent bitmaps can be resized: */ >>> + if (qcow2_truncate_bitmaps_check(bs, errp)) { >>> ret = -ENOTSUP; >>> goto fail; >>> } >>> @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>> bs->total_sectors = offset / BDRV_SECTOR_SIZE; >>> >>> /* write updated header.size */ >>> - offset = cpu_to_be64(offset); >>> + offset_be = cpu_to_be64(offset); >>> ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), >>> - &offset, sizeof(uint64_t)); >>> + &offset_be, sizeof(uint64_t)); >>> if (ret < 0) { >>> error_setg_errno(errp, -ret, "Failed to update the image size"); >>> goto fail; >>> @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>> if (ret < 0) { >>> goto fail; >>> } >>> + >>> + /* Flush bitmaps */ >>> + if (s->nb_bitmaps) { >>> + Error *local_err = NULL; >>> + >>> + /* Usually, bitmaps get resized after this call. >>> + * Force it earlier so we can make the metadata consistent. */ >>> + bdrv_dirty_bitmap_truncate(bs, offset); >>> + qcow2_flush_persistent_dirty_bitmaps(bs, &local_err); >>> + if (local_err) { >>> + ret = -EINVAL; >>> + goto fail; >>> + } >>> + } >> >> Why to flush after resize? Bitmaps will be IN_USE in the image anyway... >> >> Could we implement resize without flush first, it would be one patch + test? And then consider >> flushing in separate? >> > > If you don't flush it to disk, all calls to retrieve the bm_list begin > failing because of inconsistent metadata. > > Future calls to add bitmaps, remove bitmaps, etc will start failing. It > seemed safest and easiest to just flush the whole suite to disk. I am > trying to avoid premature optimizations at this stage, as I believe > resizes will be very infrequent events. Hmmm, understand. But I think, if we are going to allow resizes, it definitely means that IN_USE flag covers not only bitmap data, but size field too. And we must write it to specification. And it is right even if you flush here, as resize may fail after your flush, and actual disk size will remain unchanged when bitmaps are flushed with new size. And if size is covered by IN_USE, we should ignore it for IN_USE bitmaps (for IN_USE by us too). And it means, that we need check it only on qcow2_open. And not on other cases.
06.03.2019 18:52, Vladimir Sementsov-Ogievskiy wrote: > 06.03.2019 18:41, John Snow wrote: >> >> >> On 3/6/19 10:33 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 06.03.2019 2:43, John Snow wrote: >>>> Since we now load all bitmaps into memory anyway, we can just truncate them >>>> in-memory and then flush them back to disk. Just in case, we will still check >>>> and enforce that this shortcut is valid -- i.e. that any bitmap described >>>> on-disk is indeed in-memory and can be modified. >>>> >>>> If there are any inconsistent bitmaps, we refuse to allow the truncate as >>>> we do not actually load these bitmaps into memory, and it isn't safe or >>>> reasonable to attempt to truncate corrupted data. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> block/qcow2.h | 1 + >>>> block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >>>> block/qcow2.c | 27 ++++++++++++++++++++------- >>>> 3 files changed, 63 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>> index 4c1fdfcbec..b9227a72cc 100644 >>>> --- a/block/qcow2.h >>>> +++ b/block/qcow2.h >>>> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, >>>> int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, >>>> Error **errp); >>>> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >>>> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>>> void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>>> index 9373055d3b..24f280bccc 100644 >>>> --- a/block/qcow2-bitmap.c >>>> +++ b/block/qcow2-bitmap.c >>>> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) >>>> return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp); >>>> } >>>> +/* Checks to see if it's safe to resize bitmaps */ >>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp) >>>> +{ >>>> + BDRVQcow2State *s = bs->opaque; >>>> + Qcow2BitmapList *bm_list; >>>> + Qcow2Bitmap *bm; >>>> + int ret = 0; >>>> + >>>> + if (s->nb_bitmaps == 0) { >>>> + return 0; >>>> + } >>>> + >>>> + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, >>>> + s->bitmap_directory_size, false, errp); >>>> + if (bm_list == NULL) { >>>> + return -EINVAL; >>>> + } >>>> + >>>> + QSIMPLEQ_FOREACH(bm, bm_list, entry) { >>>> + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name); >>>> + if (bitmap == NULL) { >>>> + /* We rely on all bitmaps being in-memory to be able to resize them, >>>> + * Otherwise, we'd need to resize them on disk explicitly */ >>>> + error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that " >>>> + "were not loaded into memory"); >>>> + ret = -ENOTSUP; >>>> + goto out; >>>> + } >>>> + >>>> + /* The checks against readonly and busy are redundant, but certainly >>>> + * do no harm. checks against inconsistent are crucial: */ >>>> + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) { >>>> + ret = -ENOTSUP; >>>> + goto out; >>>> + } >>>> + } >>>> + >>>> +out: >>>> + bitmap_list_free(bm_list); >>>> + return ret; >>>> +} >>>> + >>>> /* store_bitmap_data() >>>> * Store bitmap to image, filling bitmap table accordingly. >>>> */ >>>> diff --git a/block/qcow2.c b/block/qcow2.c >>>> index 7fb2730f09..6fd9252494 100644 >>>> --- a/block/qcow2.c >>>> +++ b/block/qcow2.c >>>> @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>>> { >>>> BDRVQcow2State *s = bs->opaque; >>>> uint64_t old_length; >>>> - int64_t new_l1_size; >>>> + int64_t new_l1_size, offset_be; >>>> int ret; >>>> QDict *options; >>>> @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>>> goto fail; >>>> } >>>> - /* cannot proceed if image has bitmaps */ >>>> - if (s->nb_bitmaps) { >>>> - /* TODO: resize bitmaps in the image */ >>>> - error_setg(errp, "Can't resize an image which has bitmaps"); >>>> + /* Only certain persistent bitmaps can be resized: */ >>>> + if (qcow2_truncate_bitmaps_check(bs, errp)) { >>>> ret = -ENOTSUP; >>>> goto fail; >>>> } >>>> @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>>> bs->total_sectors = offset / BDRV_SECTOR_SIZE; >>>> /* write updated header.size */ >>>> - offset = cpu_to_be64(offset); >>>> + offset_be = cpu_to_be64(offset); >>>> ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), >>>> - &offset, sizeof(uint64_t)); >>>> + &offset_be, sizeof(uint64_t)); >>>> if (ret < 0) { >>>> error_setg_errno(errp, -ret, "Failed to update the image size"); >>>> goto fail; >>>> @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>>> if (ret < 0) { >>>> goto fail; >>>> } >>>> + >>>> + /* Flush bitmaps */ >>>> + if (s->nb_bitmaps) { >>>> + Error *local_err = NULL; >>>> + >>>> + /* Usually, bitmaps get resized after this call. >>>> + * Force it earlier so we can make the metadata consistent. */ >>>> + bdrv_dirty_bitmap_truncate(bs, offset); >>>> + qcow2_flush_persistent_dirty_bitmaps(bs, &local_err); >>>> + if (local_err) { >>>> + ret = -EINVAL; >>>> + goto fail; >>>> + } >>>> + } >>> >>> Why to flush after resize? Bitmaps will be IN_USE in the image anyway... >>> >>> Could we implement resize without flush first, it would be one patch + test? And then consider >>> flushing in separate? >>> >> >> If you don't flush it to disk, all calls to retrieve the bm_list begin >> failing because of inconsistent metadata. >> >> Future calls to add bitmaps, remove bitmaps, etc will start failing. It >> seemed safest and easiest to just flush the whole suite to disk. I am >> trying to avoid premature optimizations at this stage, as I believe >> resizes will be very infrequent events. > > Hmmm, understand. > > But I think, if we are going to allow resizes, it definitely means that IN_USE flag covers not > only bitmap data, but size field too. And we must write it to specification. And it is right > even if you flush here, as resize may fail after your flush, and actual disk size will remain > unchanged when bitmaps are flushed with new size. oops, you flush only on success. But, if flush fails, what would be the state of qcow2 image? resized or not? > > And if size is covered by IN_USE, we should ignore it for IN_USE bitmaps (for IN_USE by us too). > And it means, that we need check it only on qcow2_open. And not on other cases. >
On 3/6/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote: > 06.03.2019 18:41, John Snow wrote: >> >> >> On 3/6/19 10:33 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 06.03.2019 2:43, John Snow wrote: >>>> Since we now load all bitmaps into memory anyway, we can just truncate them >>>> in-memory and then flush them back to disk. Just in case, we will still check >>>> and enforce that this shortcut is valid -- i.e. that any bitmap described >>>> on-disk is indeed in-memory and can be modified. >>>> >>>> If there are any inconsistent bitmaps, we refuse to allow the truncate as >>>> we do not actually load these bitmaps into memory, and it isn't safe or >>>> reasonable to attempt to truncate corrupted data. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> block/qcow2.h | 1 + >>>> block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >>>> block/qcow2.c | 27 ++++++++++++++++++++------- >>>> 3 files changed, 63 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>> index 4c1fdfcbec..b9227a72cc 100644 >>>> --- a/block/qcow2.h >>>> +++ b/block/qcow2.h >>>> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, >>>> int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, >>>> Error **errp); >>>> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >>>> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>>> void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>>> index 9373055d3b..24f280bccc 100644 >>>> --- a/block/qcow2-bitmap.c >>>> +++ b/block/qcow2-bitmap.c >>>> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) >>>> return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp); >>>> } >>>> >>>> +/* Checks to see if it's safe to resize bitmaps */ >>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp) >>>> +{ >>>> + BDRVQcow2State *s = bs->opaque; >>>> + Qcow2BitmapList *bm_list; >>>> + Qcow2Bitmap *bm; >>>> + int ret = 0; >>>> + >>>> + if (s->nb_bitmaps == 0) { >>>> + return 0; >>>> + } >>>> + >>>> + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, >>>> + s->bitmap_directory_size, false, errp); >>>> + if (bm_list == NULL) { >>>> + return -EINVAL; >>>> + } >>>> + >>>> + QSIMPLEQ_FOREACH(bm, bm_list, entry) { >>>> + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name); >>>> + if (bitmap == NULL) { >>>> + /* We rely on all bitmaps being in-memory to be able to resize them, >>>> + * Otherwise, we'd need to resize them on disk explicitly */ >>>> + error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that " >>>> + "were not loaded into memory"); >>>> + ret = -ENOTSUP; >>>> + goto out; >>>> + } >>>> + >>>> + /* The checks against readonly and busy are redundant, but certainly >>>> + * do no harm. checks against inconsistent are crucial: */ >>>> + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) { >>>> + ret = -ENOTSUP; >>>> + goto out; >>>> + } >>>> + } >>>> + >>>> +out: >>>> + bitmap_list_free(bm_list); >>>> + return ret; >>>> +} >>>> + >>>> /* store_bitmap_data() >>>> * Store bitmap to image, filling bitmap table accordingly. >>>> */ >>>> diff --git a/block/qcow2.c b/block/qcow2.c >>>> index 7fb2730f09..6fd9252494 100644 >>>> --- a/block/qcow2.c >>>> +++ b/block/qcow2.c >>>> @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>>> { >>>> BDRVQcow2State *s = bs->opaque; >>>> uint64_t old_length; >>>> - int64_t new_l1_size; >>>> + int64_t new_l1_size, offset_be; >>>> int ret; >>>> QDict *options; >>>> >>>> @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>>> goto fail; >>>> } >>>> >>>> - /* cannot proceed if image has bitmaps */ >>>> - if (s->nb_bitmaps) { >>>> - /* TODO: resize bitmaps in the image */ >>>> - error_setg(errp, "Can't resize an image which has bitmaps"); >>>> + /* Only certain persistent bitmaps can be resized: */ >>>> + if (qcow2_truncate_bitmaps_check(bs, errp)) { >>>> ret = -ENOTSUP; >>>> goto fail; >>>> } >>>> @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>>> bs->total_sectors = offset / BDRV_SECTOR_SIZE; >>>> >>>> /* write updated header.size */ >>>> - offset = cpu_to_be64(offset); >>>> + offset_be = cpu_to_be64(offset); >>>> ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), >>>> - &offset, sizeof(uint64_t)); >>>> + &offset_be, sizeof(uint64_t)); >>>> if (ret < 0) { >>>> error_setg_errno(errp, -ret, "Failed to update the image size"); >>>> goto fail; >>>> @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>>> if (ret < 0) { >>>> goto fail; >>>> } >>>> + >>>> + /* Flush bitmaps */ >>>> + if (s->nb_bitmaps) { >>>> + Error *local_err = NULL; >>>> + >>>> + /* Usually, bitmaps get resized after this call. >>>> + * Force it earlier so we can make the metadata consistent. */ >>>> + bdrv_dirty_bitmap_truncate(bs, offset); >>>> + qcow2_flush_persistent_dirty_bitmaps(bs, &local_err); >>>> + if (local_err) { >>>> + ret = -EINVAL; >>>> + goto fail; >>>> + } >>>> + } >>> >>> Why to flush after resize? Bitmaps will be IN_USE in the image anyway... >>> >>> Could we implement resize without flush first, it would be one patch + test? And then consider >>> flushing in separate? >>> >> >> If you don't flush it to disk, all calls to retrieve the bm_list begin >> failing because of inconsistent metadata. >> >> Future calls to add bitmaps, remove bitmaps, etc will start failing. It >> seemed safest and easiest to just flush the whole suite to disk. I am >> trying to avoid premature optimizations at this stage, as I believe >> resizes will be very infrequent events. > > Hmmm, understand. > > But I think, if we are going to allow resizes, it definitely means that IN_USE flag covers not > only bitmap data, but size field too. And we must write it to specification. And it is right > even if you flush here, as resize may fail after your flush, and actual disk size will remain > unchanged when bitmaps are flushed with new size. > > And if size is covered by IN_USE, we should ignore it for IN_USE bitmaps (for IN_USE by us too). > And it means, that we need check it only on qcow2_open. And not on other cases. > Hm, so your proposal here is: - Check metadata constraints (like size) only when not marked IN_USE, (i.e. on first open) - It's fine to skip the size constraint for inconsistent bitmaps which are not loaded, because we do not load their data anyway. - subsequent bm_list loads do not need to check these constraints because the bitmaps are already in memory and we will not be using the size-related metadata. I think you are correct, but it feels a little "clever" or "tricky" and it warrants good documentation. I'll audit the codepaths to make sure this is a safe approach. (Though, even given that, I *still* want to flush bitmaps to disk on resize, just to avoid having out of date metadata on the disk -- even if we're not clearing the IN_USE bit on flush, it still seems "less wrong" to have correct headers.) To the point about resize failures: I think we do have a problem, because no matter what order we choose, we have a race condition: - flush bitmaps before resize: If we crash after the flush but before the resize, the image may be unopenable - resize before flush (this patch): If we crash before the flush, the image may be unopenable So I'll have to address that: we might not be able to avoid losing the bitmaps during resize (because of a rare failure event) but we need to make sure the image remains openable. We may want to amend bitmap loading so that it never FAILS because of bad constraints, but merely marks the image as corrupt/inconsistent. Some size issues may be recoverable but others may be "fatal" in the sense of needing to lose the bitmap. I'll respin, but I believe this patch series is absolutely crucial for 4.0... We've got until Tuesday! --js
diff --git a/block/qcow2.h b/block/qcow2.h index 4c1fdfcbec..b9227a72cc 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 9373055d3b..24f280bccc 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp); } +/* Checks to see if it's safe to resize bitmaps */ +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp) +{ + BDRVQcow2State *s = bs->opaque; + Qcow2BitmapList *bm_list; + Qcow2Bitmap *bm; + int ret = 0; + + if (s->nb_bitmaps == 0) { + return 0; + } + + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, + s->bitmap_directory_size, false, errp); + if (bm_list == NULL) { + return -EINVAL; + } + + QSIMPLEQ_FOREACH(bm, bm_list, entry) { + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name); + if (bitmap == NULL) { + /* We rely on all bitmaps being in-memory to be able to resize them, + * Otherwise, we'd need to resize them on disk explicitly */ + error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that " + "were not loaded into memory"); + ret = -ENOTSUP; + goto out; + } + + /* The checks against readonly and busy are redundant, but certainly + * do no harm. checks against inconsistent are crucial: */ + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) { + ret = -ENOTSUP; + goto out; + } + } + +out: + bitmap_list_free(bm_list); + return ret; +} + /* store_bitmap_data() * Store bitmap to image, filling bitmap table accordingly. */ diff --git a/block/qcow2.c b/block/qcow2.c index 7fb2730f09..6fd9252494 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, { BDRVQcow2State *s = bs->opaque; uint64_t old_length; - int64_t new_l1_size; + int64_t new_l1_size, offset_be; int ret; QDict *options; @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, goto fail; } - /* cannot proceed if image has bitmaps */ - if (s->nb_bitmaps) { - /* TODO: resize bitmaps in the image */ - error_setg(errp, "Can't resize an image which has bitmaps"); + /* Only certain persistent bitmaps can be resized: */ + if (qcow2_truncate_bitmaps_check(bs, errp)) { ret = -ENOTSUP; goto fail; } @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, bs->total_sectors = offset / BDRV_SECTOR_SIZE; /* write updated header.size */ - offset = cpu_to_be64(offset); + offset_be = cpu_to_be64(offset); ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), - &offset, sizeof(uint64_t)); + &offset_be, sizeof(uint64_t)); if (ret < 0) { error_setg_errno(errp, -ret, "Failed to update the image size"); goto fail; @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, if (ret < 0) { goto fail; } + + /* Flush bitmaps */ + if (s->nb_bitmaps) { + Error *local_err = NULL; + + /* Usually, bitmaps get resized after this call. + * Force it earlier so we can make the metadata consistent. */ + bdrv_dirty_bitmap_truncate(bs, offset); + qcow2_flush_persistent_dirty_bitmaps(bs, &local_err); + if (local_err) { + ret = -EINVAL; + goto fail; + } + } + ret = 0; fail: qemu_co_mutex_unlock(&s->lock);
Since we now load all bitmaps into memory anyway, we can just truncate them in-memory and then flush them back to disk. Just in case, we will still check and enforce that this shortcut is valid -- i.e. that any bitmap described on-disk is indeed in-memory and can be modified. If there are any inconsistent bitmaps, we refuse to allow the truncate as we do not actually load these bitmaps into memory, and it isn't safe or reasonable to attempt to truncate corrupted data. Signed-off-by: John Snow <jsnow@redhat.com> --- block/qcow2.h | 1 + block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 27 ++++++++++++++++++++------- 3 files changed, 63 insertions(+), 7 deletions(-)