Am 24.06.2016 um 13:10 hat zhangzhiming geschrieben:
> qcow2 can’t be resized while there is a snapshot in qcow2 image but in version
> 3 image of qcow2,
> each disk size of snapshot is stored in the image, so we can resize image even
> through this are some snapshots
> since version 3 image. the function of resize nearly finished by others, i
> just do very little job.
>
>
> Signed-off-by: zhangzhiming <zhangzhimin...@meituan.com>
If you can, please try to use 'git send-email' to send the patch. 'git
am' has trouble importing your patch correctly.
> --- a/block.c
> +++ b/block.c
> @@ -2586,6 +2586,25 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
> return ret;
> }
>
> +int bdrv_apply_snapshot(BlockDriverState *bs, const char *snapshot_id,
> + uint64_t snapshot_size)
> +{
> + int ret = bdrv_snapshot_goto(bs, snapshot_id);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + ret = refresh_total_sectors(bs, snapshot_size >> BDRV_SECTOR_BITS);
> + if (ret < 0) {
> + return ret;
> + }
> + bdrv_dirty_bitmap_truncate(bs);
> + if (bs->blk) {
> + blk_dev_resize_cb(bs->blk);
> + }
> + return ret;
> +}
Am I missing something or is this function unused?
> +int shrink_l1_table(BlockDriverState *bs, int64_t new_l1_size)
> +{
> + BDRVQcow2State *s = bs->opaque;
> + int64_t old_l1_size = s->l1_size;
> + s->l1_size = new_l1_size;
> + int ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> + s->l1_size, 1);
> + if (ret < 0) {
> + return ret;
If we return here, we continue with the new_l1_size even though the
header hasn't been updated yet.
> + }
> +
> + s->l1_size = old_l1_size;
> + ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> + s->l1_size, -1);
> + if (ret < 0) {
> + return ret;
> + }
> + s->l1_size = new_l1_size;
Also this whole procedure feels very wasteful, we first update all
clusters to increase the refcount and clear the COPIED flag and
afterwards set decrease the refcount and set the flag again.
Wouldn't it make more sense to have a function that drops just part of
the image?
In fact, as we're operating on the active L1 table, couldn't we just
reuse qcow2_discard_clusters() for the data clusters? L2 clusters aren't
discarded currently, but adding that shouldn't be hard.
> + uint32_t be_l1_size = cpu_to_be32(s->l1_size);
> + ret = bdrv_pwrite_sync(bs->file->bs, offsetof(QCowHeader, l1_size),
> + &be_l1_size, sizeof(be_l1_size));
> + if (ret < 0) {
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
> bool exact_size)
> {
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 242fb21..6dd6769 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -478,13 +478,6 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char
> *snapshot_id)
> }
> sn = &s->snapshots[snapshot_index];
>
> - if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
> - error_report("qcow2: Loading snapshots with different disk "
> - "size is not implemented");
> - ret = -ENOTSUP;
> - goto fail;
> - }
> -
> /*
> * Make sure that the current L1 table is big enough to contain the whole
> * L1 table of the snapshot. If the snapshot L1 table is smaller, the
> @@ -506,8 +499,8 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char
> *snapshot_id)
> * Decrease the refcount referenced by the old one only when the L1
> * table is overwritten.
> */
> - sn_l1_table = g_try_malloc0(cur_l1_bytes);
> - if (cur_l1_bytes && sn_l1_table == NULL) {
> + sn_l1_table = g_try_malloc0(sn_l1_bytes);
> + if (sn_l1_bytes && sn_l1_table == NULL) {
> ret = -ENOMEM;
> goto fail;
> }
> @@ -525,17 +518,34 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char
> *snapshot_id)
> }
>
> ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L1,
> - s->l1_table_offset, cur_l1_bytes);
> + s->l1_table_offset, sn_l1_bytes);
> if (ret < 0) {
> goto fail;
> }
>
> ret = bdrv_pwrite_sync(bs->file->bs, s->l1_table_offset, sn_l1_table,
> - cur_l1_bytes);
> + sn_l1_bytes);
> + if (ret < 0) {
> + goto fail;
> + }
At this point, the l1 size in the header on disk still has the old size.
If qemu crashes here, the image is interpreted to have an L1 table that
consists of one half from the snapshot and another half from the old
active L1 table.
Should we instead zero out the part between new and old size?
> + /* write updated header.size */
> + uint64_t be_disk_size = cpu_to_be64(sn->disk_size);
> + ret = bdrv_pwrite_sync(bs->file->bs, offsetof(QCowHeader, size),
> + &be_disk_size, sizeof(uint64_t));
> if (ret < 0) {
> goto fail;
> }
>
> + uint32_t be_l1_size = cpu_to_be32(sn->l1_size);
> + ret = bdrv_pwrite_sync(bs->file->bs, offsetof(QCowHeader, l1_size),
> + &be_l1_size, sizeof(be_l1_size));
> + if (ret < 0) {
> + goto fail;
> + }
We should probably atomically write a new header with
qcow2_update_header(). Failure at this point results in an inconsistent
image.
> +
> + s->l1_vm_state_index = sn->l1_size;
I'm doubtful of this. The snapshot that we're loading already has a VM
state included in its L1 table. This probably sets a too high value.
Did you test this with savevm/loadvm?
> /*
> * Decrease refcount of clusters of current L1 table.
> *
> @@ -553,7 +563,8 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char
> *snapshot_id)
> * Now update the in-memory L1 table to be in sync with the on-disk one.
> We
> * need to do this even if updating refcounts failed.
> */
> - for(i = 0;i < s->l1_size; i++) {
> + memset(s->l1_table, 0, s->l1_size * sizeof(uint64_t));
> + for (i = 0; i < sn->l1_size; i++) {
> s->l1_table[i] = be64_to_cpu(sn_l1_table[i]);
> }
>
> @@ -564,6 +575,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char
> *snapshot_id)
> g_free(sn_l1_table);
> sn_l1_table = NULL;
>
> + s->l1_size = sn->l1_size;
> /*
> * Update QCOW_OFLAG_COPIED in the active L1 table (it may have changed
> * when we decreased the refcount of the old snapshot.
> @@ -676,6 +688,7 @@ int qcow2_snapshot_list(BlockDriverState *bs,
> QEMUSnapshotInfo **psn_tab)
> sn_info->date_sec = sn->date_sec;
> sn_info->date_nsec = sn->date_nsec;
> sn_info->vm_clock_nsec = sn->vm_clock_nsec;
> + sn_info->disk_size = sn->disk_size;
> }
> *psn_tab = sn_tab;
> return s->nb_snapshots;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6f5fb81..612a534 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2495,22 +2495,33 @@ static int qcow2_truncate(BlockDriverState *bs,
> int64_t
> offset)
> return -EINVAL;
> }
>
> - /* cannot proceed if image has snapshots */
> - if (s->nb_snapshots) {
> - error_report("Can't resize an image which has snapshots");
> + bool v3_truncate = (s->qcow_version == 3);
> +
> + /* cannot proceed if image has snapshots and qcow_version is not 3 */
> + if (!v3_truncate && s->nb_snapshots) {
> + error_report("Can't resize an image which has snapshots and "
> + "qcow_version is not 3");
Maybe rephrasing this as "requires an compat=1.1 image" would be easier
to understand for a user who doesn't know the qcow2 internals.
> return -ENOTSUP;
> }
>
> - /* shrinking is currently not supported */
> - if (offset < bs->total_sectors * 512) {
> - error_report("qcow2 doesn't support shrinking images yet");
> + /* shrinking is supported from version 3 */
> + if (!v3_truncate && offset < bs->total_sectors * 512) {
> + error_report("qcow2 doesn't support shrinking images yet while"
> + " qcow_version is not 3");
> return -ENOTSUP;
> }
>
> new_l1_size = size_to_l1(s, offset);
> - ret = qcow2_grow_l1_table(bs, new_l1_size, true);
> - if (ret < 0) {
> - return ret;
> + if (offset < bs->total_sectors * 512) {
> + ret = shrink_l1_table(bs, new_l1_size);
> + if (ret < 0) {
> + return ret;
> + }
> + } else {
> + ret = qcow2_grow_l1_table(bs, new_l1_size, true);
> + if (ret < 0) {
> + return ret;
> + }
> }
Kevin
