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 <[email protected]> >>>> --- >>>> 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. > -- Best regards, Vladimir
