Skip to contentSkip to conversation

qcow2: Patch for shrinking qcow2 disk image

PATCH Superseded Jun Li 2 participants 5 messages Oct 14, 2014 – Oct 22, 2014 Block layer

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Jun Li

author
#0 lore raw

v4 cover qcow2: Patch for shrinking qcow2 disk image

v4:
Add deal with COW clusters in l2 table. When using COW, some of (l2_entry >>
s->cluster_bits) will larger than s->refcount_table_size, so need to discard this l2_entry.

v3:
Fixed host cluster leak.

A simple testing:

Step 1,

# /opt/qemu-git-arm/bin/qemu-img info /home/lijun/Work/tmp/shrink.qcow2
image: /home/lijun/Work/tmp/shrink.qcow2
file format: qcow2
virtual size: 2.0G (2147483648 bytes)
disk size: 2.0G
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false
    corrupt: false

Step 2,

# /opt/qemu-git-arm/bin/qemu-img resize /home/lijun/Work/tmp/shrink.qcow2 -100M
Image resized.

Step 3,

# /opt/qemu-git-arm/bin/qemu-img info /home/lijun/Work/tmp/shrink.qcow2
image: /home/lijun/Work/tmp/shrink.qcow2
file format: qcow2
virtual size: 1.9G (2042626048 bytes)
disk size: 1.9G
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false
    corrupt: false

Step 4,

# /opt/qemu-git-arm/bin/qemu-img check /home/lijun/Work/tmp/shrink.qcow2
No errors were found on the image.
24576/31168 = 78.85% allocated, 0.80% fragmented, 0.00% compressed clusters
Image end offset: 1631911936

As above show, in step 1, "disk size" and "virtual size" are all 2.0G. After Step 3, "disk size" and "virtual size" are all 1.9G.

BTW, above is also a simple testing. I will submit a test case for qemu-iotests later. But I am not so familar with howto write qemu-iotests now.

In file block/qcow2.c, just using ftruncate to fix host cluster leak.

In file block/qcow2-cluster.c, just re-copy qcow2_grow_l1_table to realize qcow2_shrink_l1_and_l2_table.

In file block/qcow2-refcount.c, also update the realization to handle self-describing refcount blocks in function update_refcount.

Thanks.

Jun Li (2):
qcow2: Add qcow2_shrink_l1_and_l2_table for qcow2 shrinking
qcow2: add update refcount table realization for update_refcount

 block/qcow2-cluster.c  | 186 +++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2-refcount.c |  46 +++++++++++-
 block/qcow2.c          |  40 +++++++++--
 block/qcow2.h          |   2 +
 4 files changed, 269 insertions(+), 5 deletions(-)
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH v4 1/2 qcow2: Add qcow2_shrink_l1_and_l2_table for qcow2 shrinking

This patch is the realization of new function qcow2_shrink_l1_and_l2_table. This function will shrink/discard l1 and l2 table when do qcow2 shrinking.

Signed-off-by Jun Li <[email protected]>

---
v4:
Add deal with COW clusters in l2 table. When using COW, some of (l2_entry >>
s->cluster_bits) will larger than s->refcount_table_size, so need to discard this l2_entry.
v3:
Fixed host cluster leak.

---
 block/qcow2-cluster.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c         |  40 +++++++++--
 block/qcow2.h         |   2 +
 3 files changed, 224 insertions(+), 4 deletions(-)
block/qcow2-cluster.c +186 -0
index f7dd8c0..0664b8a 100644
@@ -29,6 +29,9 @@
 #include "block/qcow2.h"
 #include "trace.h"
 
+static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
+                   uint64_t **l2_table);
+
 int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
                         bool exact_size)
 {

@@ -135,6 +138,189 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
     return ret;
 }
 
+int qcow2_shrink_l1_and_l2_table(BlockDriverState *bs, uint64_t new_l1_size,
+                                 int new_l2_index, bool exact_size)
+{
+    BDRVQcowState *s = bs->opaque;
+    int new_l1_size2, ret, i;
+    uint64_t *new_l1_table;
+    int64_t new_l1_table_offset;
+    int64_t old_l1_table_offset, old_l1_size;
+    uint8_t data[12];
+
+    new_l1_size2 = sizeof(uint64_t) * new_l1_size;
+    new_l1_table = qemu_try_blockalign(bs->file,
+                                       align_offset(new_l1_size2, 512));
+    if (new_l1_table == NULL) {
+        return -ENOMEM;
+    }
+    memset(new_l1_table, 0, align_offset(new_l1_size2, 512));
+
+    /* shrinking l1 table */
+    memcpy(new_l1_table, s->l1_table, new_l1_size2);
+
+    /* write new table (align to cluster) */
+    BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);
+    new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
+    if (new_l1_table_offset < 0) {
+        qemu_vfree(new_l1_table);
+        return new_l1_table_offset;
+    }
+
+    ret = qcow2_cache_flush(bs, s->refcount_block_cache);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* the L1 position has not yet been updated, so these clusters must
+     * indeed be completely free */
+    ret = qcow2_pre_write_overlap_check(bs, 0, new_l1_table_offset,
+                                        new_l1_size2);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_WRITE_TABLE);
+
+    for (i = 0; i < new_l1_size; i++) {
+        new_l1_table[i] = cpu_to_be64(new_l1_table[i]);
+    }
+
+    ret = bdrv_pwrite_sync(bs->file, new_l1_table_offset,
+                           new_l1_table, new_l1_size2);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    for (i = 0; i < new_l1_size; i++) {
+        new_l1_table[i] = be64_to_cpu(new_l1_table[i]);
+    }
+
+    /* set new table */
+    BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ACTIVATE_TABLE);
+    cpu_to_be32w((uint32_t *)data, new_l1_size);
+    stq_be_p(data + 4, new_l1_table_offset);
+    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_size),
+                           data, sizeof(data));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    old_l1_table_offset = s->l1_table_offset;
+    s->l1_table_offset = new_l1_table_offset;
+    uint64_t *old_l1_table = s->l1_table;
+    s->l1_table = new_l1_table;
+    old_l1_size = s->l1_size;
+    s->l1_size = new_l1_size;
+
+    int num = old_l1_size - s->l1_size;
+
+    while (num >= 0) {
+        uint64_t l2_offset;
+        int ret;
+        uint64_t *l2_table, l2_entry;
+        int last_free_cluster = 0;
+
+        l2_offset = old_l1_table[s->l1_size + num - 1] & L1E_OFFSET_MASK;
+        if (l2_offset == 0) {
+            goto retry;
+        }
+
+        if (num == 0) {
+            if (new_l2_index == 0) {
+                goto retry;
+            }
+            last_free_cluster = new_l2_index + 1;
+        }
+
+        /* load l2_table into cache */
+        ret = l2_load(bs, l2_offset, &l2_table);
+
+        if (ret < 0) {
+            goto fail;
+        }
+
+        for (i = s->l2_size; i > 0; i--) {
+            l2_entry = be64_to_cpu(l2_table[i - 1]);
+
+            /* Deal with COW clusters in l2 table */
+            if ((i <= last_free_cluster)) {
+                if ((l2_entry >> s->cluster_bits) >
+                 s->refcount_table_size) {
+                    goto lable1;
+                }
+                continue;
+            }
+
+            switch (qcow2_get_cluster_type(l2_entry)) {
+            case QCOW2_CLUSTER_UNALLOCATED:
+                if (!bs->backing_hd) {
+                    continue;
+                }
+                break;
+
+            case QCOW2_CLUSTER_ZERO:
+                continue;
+
+            case QCOW2_CLUSTER_NORMAL:
+            case QCOW2_CLUSTER_COMPRESSED:
+                break;
+
+            default:
+                abort();
+            }
+
+        lable1:
+            qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
+
+            uint64_t data64;
+            if (s->qcow_version >= 3) {
+                l2_table[i - 1] = cpu_to_be64(QCOW_OFLAG_ZERO);
+                data64 = cpu_to_be64(QCOW_OFLAG_ZERO);
+            } else {
+                l2_table[i - 1] = cpu_to_be64(0);
+                data64 = cpu_to_be64(0);
+            }
+
+            /* before discard the specify item of l2_entry,
+             * set this entry with 0.
+             */
+            ret = bdrv_pwrite_sync(bs->file,
+                                   l2_offset + (i-1) * sizeof(uint64_t),
+                                   &data64, sizeof(data64));
+            if (ret < 0) {
+                goto fail;
+            }
+
+            /* Then decrease the refcount */
+            qcow2_free_any_clusters(bs, l2_entry, 1, QCOW2_DISCARD_MAX);
+        }
+
+        ret = qcow2_cache_put(bs, s->l2_table_cache, (void **) &l2_table);
+        if (ret < 0) {
+            goto fail;
+        }
+        if (last_free_cluster == 0) {
+            qemu_vfree(l2_table);
+            qcow2_free_clusters(bs, l2_offset, s->cluster_size - 1,
+                                QCOW2_DISCARD_OTHER);
+        }
+    retry:
+        num--;
+    }
+
+    qemu_vfree(old_l1_table);
+
+    qcow2_free_clusters(bs, old_l1_table_offset, old_l1_size * sizeof(uint64_t),
+                        QCOW2_DISCARD_OTHER);
+    return 0;
+ fail:
+    qemu_vfree(new_l1_table);
+    qcow2_free_clusters(bs, new_l1_table_offset, new_l1_size2,
+                        QCOW2_DISCARD_OTHER);
+    return ret;
+}
+
 /*
  * l2_load
  *
block/qcow2.c +36 -4
index fb28493..bed1558 100644
@@ -2097,7 +2097,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
     int64_t new_l1_size;
     int ret;
 
-    if (offset & 511) {
+    if (offset <= 0 || offset & 511) {
         error_report("The new size must be a multiple of 512");
         return -EINVAL;
     }

@@ -2108,10 +2108,42 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     }
 
-    /* shrinking is currently not supported */
+    /* shrinking image */
     if (offset < bs->total_sectors * 512) {
-        error_report("qcow2 doesn't support shrinking images yet");
-        return -ENOTSUP;
+
+        /* As l1 table, l2 table, refcount table, refcount block table
+         * and file header of the qcow2 image need to use some clusters,
+         * so should subtract these metadata from offset.
+         */
+        int64_t nb_l1 = (s->l1_size >> 3) >> s->cluster_bits;
+        int64_t nb_l2 = offset / (s->l2_size << s->cluster_bits);
+        int64_t nb_refcount_block_table = offset / ((s->cluster_size >>
+                                          REFCOUNT_SHIFT) << s->cluster_bits);
+        int64_t nb_refcount_table = (nb_refcount_block_table << 3) >>
+                                     s->cluster_bits;
+        int64_t tatol_nb = nb_l2 + nb_l1 + nb_refcount_block_table +
+                           nb_refcount_table + 1;
+        int64_t offset_for_shrink = offset - (tatol_nb << s->cluster_bits);
+        int new_l2_index = offset_to_l2_index(s, offset_for_shrink);
+
+        new_l1_size = size_to_l1(s, offset_for_shrink);
+        ret = qcow2_shrink_l1_and_l2_table(bs, new_l1_size, new_l2_index, true);
+        if (ret < 0) {
+            return ret;
+        }
+
+        int64_t actual_size = bdrv_get_allocated_file_size(bs);
+
+        if (offset < actual_size) {
+            int fd = qemu_open(bs->filename, O_WRONLY,
+                               O_APPEND);
+            if (fd >= 0) {
+                if (ftruncate(fd, offset) < 0) {
+                    return -errno;
+                }
+                qemu_close(fd);
+            }
+        }
     }
 
     new_l1_size = size_to_l1(s, offset);
block/qcow2.h +2 -0
index 7d61e61..a9c8acb 100644
@@ -516,6 +516,8 @@ int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
 /* qcow2-cluster.c functions */
 int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
                         bool exact_size);
+int qcow2_shrink_l1_and_l2_table(BlockDriverState *bs, uint64_t new_l1_size,
+                                 int new_l2_index, bool exact_size);
 int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
 void qcow2_l2_cache_reset(BlockDriverState *bs);
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
Signature · 1 line
1.9.3
#2 lore raw

Re: [Qemu-devel] [PATCH v4 1/2] qcow2: Add qcow2_shrink_l1_and_l2_table for qcow2 shrinking

Quoted 36 lines · Jun Li

This patch is the realization of new function qcow2_shrink_l1_and_l2_table. This function will shrink/discard l1 and l2 table when do qcow2 shrinking.

Signed-off-by Jun Li <[email protected]>

---
v4:
Add deal with COW clusters in l2 table. When using COW, some of (l2_entry >>
s->cluster_bits) will larger than s->refcount_table_size, so need to discard this l2_entry.
v3:
Fixed host cluster leak.

---
  block/qcow2-cluster.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++++++
  block/qcow2.c         |  40 +++++++++--
  block/qcow2.h         |   2 +
  3 files changed, 224 insertions(+), 4 deletions(-)
block/qcow2-cluster.c +4 -0
index f7dd8c0..0664b8a 100644
@@ -29,6 +29,9 @@
  #include "block/qcow2.h"
  #include "trace.h"
 
+static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
+                   uint64_t **l2_table);
+
  int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
                          bool exact_size)
  {

@@ -135,6 +138,189 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
      return ret;
  }
 
+int qcow2_shrink_l1_and_l2_table(BlockDriverState *bs, uint64_t new_l1_size,

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 151

Quoted 10 lines
block/qcow2-cluster.c +10 -0
+                                 int new_l2_index, bool exact_size)
+{
+    BDRVQcowState *s = bs->opaque;
+    int new_l1_size2, ret, i;
+    uint64_t *new_l1_table;
+    int64_t new_l1_table_offset;
+    int64_t old_l1_table_offset, old_l1_size;
+    uint8_t data[12];
+
+    new_l1_size2 = sizeof(uint64_t) * new_l1_size;

new_l1_size is uint64_t, new_l1_size2 is int. Not a good idea.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 153

Quoted 2 lines
block/qcow2-cluster.c +2 -0
+    new_l1_table = qemu_try_blockalign(bs->file,
+                                       align_offset(new_l1_size2, 512));

As long as with 512 you mean the sector size, s/512/BDRV_SECTOR_SIZE/.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 157

Quoted 4 lines
block/qcow2-cluster.c +4 -0
+    if (new_l1_table == NULL) {
+        return -ENOMEM;
+    }
+    memset(new_l1_table, 0, align_offset(new_l1_size2, 512));

Same here.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 160

Quoted 3 lines
block/qcow2-cluster.c +3 -0
+
+    /* shrinking l1 table */
+    memcpy(new_l1_table, s->l1_table, new_l1_size2);

Please add an assert(new_l1_size <= s->l1_size) before this.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 163

Quoted 3 lines
block/qcow2-cluster.c +3 -0
+
+    /* write new table (align to cluster) */
+    BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);

Well, it's not really growing, but fine.

Quoted 13 lines
block/qcow2-cluster.c +13 -0
+    new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
+    if (new_l1_table_offset < 0) {
+        qemu_vfree(new_l1_table);
+        return new_l1_table_offset;
+    }
+
+    ret = qcow2_cache_flush(bs, s->refcount_block_cache);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* the L1 position has not yet been updated, so these clusters must
+     * indeed be completely free */

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 186

Quoted 10 lines
block/qcow2-cluster.c +10 -0
+    ret = qcow2_pre_write_overlap_check(bs, 0, new_l1_table_offset,
+                                        new_l1_size2);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_WRITE_TABLE);
+
+    for (i = 0; i < new_l1_size; i++) {
+        new_l1_table[i] = cpu_to_be64(new_l1_table[i]);

You could have used cpu_to_be64s(&new_l1_table[i]) here.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 196

Quoted 10 lines
block/qcow2-cluster.c +10 -0
+    }
+
+    ret = bdrv_pwrite_sync(bs->file, new_l1_table_offset,
+                           new_l1_table, new_l1_size2);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    for (i = 0; i < new_l1_size; i++) {
+        new_l1_table[i] = be64_to_cpu(new_l1_table[i]);

Again, cpu_to_be64s() is an alternative.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 202

Quoted 6 lines
block/qcow2-cluster.c +6 -0
+    }
+
+    /* set new table */
+    BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ACTIVATE_TABLE);
+    cpu_to_be32w((uint32_t *)data, new_l1_size);
+    stq_be_p(data + 4, new_l1_table_offset);

Why not cpu_to_be64w()? I find it easier to read and it definitely fits in better.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 211

Quoted 9 lines
block/qcow2-cluster.c +9 -0
+    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_size),
+                           data, sizeof(data));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    old_l1_table_offset = s->l1_table_offset;
+    s->l1_table_offset = new_l1_table_offset;
+    uint64_t *old_l1_table = s->l1_table;

Declarations have to be at the start of the block.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 216

Quoted 5 lines
block/qcow2-cluster.c +5 -0
+    s->l1_table = new_l1_table;
+    old_l1_size = s->l1_size;
+    s->l1_size = new_l1_size;
+
+    int num = old_l1_size - s->l1_size;

Same here.

Quoted 14 lines
block/qcow2-cluster.c +14 -0
+
+    while (num >= 0) {
+        uint64_t l2_offset;
+        int ret;
+        uint64_t *l2_table, l2_entry;
+        int last_free_cluster = 0;
+
+        l2_offset = old_l1_table[s->l1_size + num - 1] & L1E_OFFSET_MASK;
+        if (l2_offset == 0) {
+            goto retry;
+        }
+
+        if (num == 0) {
+            if (new_l2_index == 0) {

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 240

Quoted 10 lines
block/qcow2-cluster.c +10 -0
+                goto retry;
+            }
+            last_free_cluster = new_l2_index + 1;
+        }
+
+        /* load l2_table into cache */
+        ret = l2_load(bs, l2_offset, &l2_table);
+
+        if (ret < 0) {
+            goto fail;

Too bad we're freeing new_l1_table in fail. We should be freeing old_l1_table.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 244

Quoted 4 lines
block/qcow2-cluster.c +4 -0
+        }
+
+        for (i = s->l2_size; i > 0; i--) {
+            l2_entry = be64_to_cpu(l2_table[i - 1]);

Okay... I'd probably have gone with for (i = s->l2_size - 1; i >= 0; i--) and then l2_entry = be64_to_cpu(l2_table[i]).

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 247

Quoted 3 lines
block/qcow2-cluster.c +3 -0
+
+            /* Deal with COW clusters in l2 table */
+            if ((i <= last_free_cluster)) {

I'd like some comments on what last_free_cluster is. Also, the inner parentheses should be removed.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 249

Quoted 2 lines
block/qcow2-cluster.c +2 -0
+                if ((l2_entry >> s->cluster_bits) >
+                 s->refcount_table_size) {

Misalignment.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 250

Quoted 1 line
block/qcow2-cluster.c +1 -0
+                    goto lable1;

It's "label", and "label1" is not a very helpful name.

Quoted 27 lines
block/qcow2-cluster.c +27 -0
+                }
+                continue;
+            }
+
+            switch (qcow2_get_cluster_type(l2_entry)) {
+            case QCOW2_CLUSTER_UNALLOCATED:
+                if (!bs->backing_hd) {
+                    continue;
+                }
+                break;
+
+            case QCOW2_CLUSTER_ZERO:
+                continue;
+
+            case QCOW2_CLUSTER_NORMAL:
+            case QCOW2_CLUSTER_COMPRESSED:
+                break;
+
+            default:
+                abort();
+            }
+
+        lable1:
+            qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
+
+            uint64_t data64;
+            if (s->qcow_version >= 3) {

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 287

Quoted 10 lines
block/qcow2-cluster.c +10 -0
+                l2_table[i - 1] = cpu_to_be64(QCOW_OFLAG_ZERO);
+                data64 = cpu_to_be64(QCOW_OFLAG_ZERO);
+            } else {
+                l2_table[i - 1] = cpu_to_be64(0);
+                data64 = cpu_to_be64(0);
+            }
+
+            /* before discard the specify item of l2_entry,
+             * set this entry with 0.
+             */

Why?

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 290

Quoted 3 lines
block/qcow2-cluster.c +3 -0
+            ret = bdrv_pwrite_sync(bs->file,
+                                   l2_offset + (i-1) * sizeof(uint64_t),
+                                   &data64, sizeof(data64));

Why aren't you just going through the cache?

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 297

Quoted 7 lines
block/qcow2-cluster.c +7 -0
+            if (ret < 0) {
+                goto fail;
+            }
+
+            /* Then decrease the refcount */
+            qcow2_free_any_clusters(bs, l2_entry, 1, QCOW2_DISCARD_MAX);
+        }

So I think what this code tried to achieve was freeing all data clusters referenced from L2 tables which are about to be dropped. Since this will change the data in these clusters anyway (and these clusters are not guest-accessible any more, so we don't have to care), just mark all of these L2 entries unallocated, don't bother with making them zero clusters.

Simply marking the image dirty would be much easier, however. The problem is that this doesn't work for compat=0.10. I'd be fine (and by that, I mean I'd like it) with supporting shrinking only for compat=1.1 images.

Quoted 16 lines
block/qcow2-cluster.c +16 -0
+
+        ret = qcow2_cache_put(bs, s->l2_table_cache, (void **) &l2_table);
+        if (ret < 0) {
+            goto fail;
+        }
+        if (last_free_cluster == 0) {
+            qemu_vfree(l2_table);
+            qcow2_free_clusters(bs, l2_offset, s->cluster_size - 1,
+                                QCOW2_DISCARD_OTHER);
+        }
+    retry:
+        num--;
+    }
+
+    qemu_vfree(old_l1_table);
+

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 323

Quoted 10 lines
block/qcow2-cluster.c +10 -0
+    qcow2_free_clusters(bs, old_l1_table_offset, old_l1_size * sizeof(uint64_t),
+                        QCOW2_DISCARD_OTHER);
+    return 0;
+ fail:
+    qemu_vfree(new_l1_table);
+    qcow2_free_clusters(bs, new_l1_table_offset, new_l1_size2,
+                        QCOW2_DISCARD_OTHER);
+    return ret;
+}
+

So, all in all, I'd strongly propose just not supporting shrinking for compat=0.10; and for compat=1.1, we can simply mark the image dirty and then shrink the L1 table.

Quoted 8 lines
block/qcow2-cluster.c +0 -0
  /*
   * l2_load
   *
block/qcow2.c +0 -0
index fb28493..bed1558 100644

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2101

Quoted 6 lines
block/qcow2.c +1 -1
@@ -2097,7 +2097,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
      int64_t new_l1_size;
      int ret;
 
-    if (offset & 511) {
+    if (offset <= 0 || offset & 511) {
          error_report("The new size must be a multiple of 512");

This error message is inappropriate for offset <= 0.

I think an assert(offset > 0) should be fine, too. bdrv_truncate() may not be called with a negative offset, as far as I know.

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2113

Quoted 12 lines
block/qcow2.c +2 -3
          return -EINVAL;
      }

@@ -2108,10 +2108,42 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
          return -ENOTSUP;
      }
 
-    /* shrinking is currently not supported */
+    /* shrinking image */
      if (offset < bs->total_sectors * 512) {
-        error_report("qcow2 doesn't support shrinking images yet");
-        return -ENOTSUP;
+

Spurious empty line?

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2118

Quoted 5 lines
block/qcow2.c +5 -0
+        /* As l1 table, l2 table, refcount table, refcount block table
+         * and file header of the qcow2 image need to use some clusters,
+         * so should subtract these metadata from offset.
+         */
+        int64_t nb_l1 = (s->l1_size >> 3) >> s->cluster_bits;

s->l1_size is the number of entries in the L1 table. s->cluster_bits can be used to convert a number of bytes to a number of clusters. I guess what you want is (s->l1_size << 3) >> s->cluster_bits. But since s->l1_size << 3 does not have to a multiple of the cluster size, I think DIV_ROUND_UP(s->l1_size * sizeof(uint64_t), s->cluster_size) may be best (or DIV_ROUND_UP((uint64_t)s->l1_size * sizeof(uint64_t), s->cluster_size), since s->l1_size is only an int).

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2119

Quoted 1 line
block/qcow2.c +1 -0
+        int64_t nb_l2 = offset / (s->l2_size << s->cluster_bits);

This should be DIV_ROUND_UP(offset, s->l2_size << s->cluster_bits). Maybe even DIV_ROUND_UP(offset, (uint64_t)s->l2_size << s->cluster_bits) since s->l2_size is only an int.

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2123

Quoted 4 lines
block/qcow2.c +4 -0
+        int64_t nb_refcount_block_table = offset / ((s->cluster_size >>
+                                          REFCOUNT_SHIFT) << s->cluster_bits);
+        int64_t nb_refcount_table = (nb_refcount_block_table << 3) >>
+                                     s->cluster_bits;

The number of refblocks and the reftable size required does not depend on the offset which is the guest-visible image size. Instead, it depends on the host image size. Since the refblock count and the reftable size directly influences that host image size, this is not a trivial matter. See qcow2_create2() in the "if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC)" block for an example. Note that that function is not necessarily correct (a truly correct function for this should not be using floating point calculation, at the very least), but it was enough for that case. In this case however, we should do it right if we want to shrink the refcount table and reduce the number of refcount blocks.

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2124

Quoted 1 line
block/qcow2.c +1 -0
+        int64_t tatol_nb = nb_l2 + nb_l1 + nb_refcount_block_table +

Should be "total_nb".

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2126

Quoted 2 lines
block/qcow2.c +2 -0
+                           nb_refcount_table + 1;
+        int64_t offset_for_shrink = offset - (tatol_nb << s->cluster_bits);

offset is not the host image size but the guest disk size. It should be "file_size = offset + (total_nb << s->cluster_bits)" or something like that.

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2127

Quoted 1 line
block/qcow2.c +1 -0
+        int new_l2_index = offset_to_l2_index(s, offset_for_shrink);

This would then be s/offset_for_shrink/offset/. Well, at least it should be. It would not be, because this new_l2_index is something strange, probably it's supposed to be the number of L2 indices to be culled. It shouldn't be. I think you want to pass the number of L2 indices to keep to qcow2_shrink_l1_and_l2_table(), that is, offset_to_l2_index(s, offset).

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2129

Quoted 2 lines
block/qcow2.c +2 -0
+
+        new_l1_size = size_to_l1(s, offset_for_shrink);

s/offset_for_shrink/offset/.

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2137

Quoted 8 lines
block/qcow2.c +8 -0
+        ret = qcow2_shrink_l1_and_l2_table(bs, new_l1_size, new_l2_index, true);
+        if (ret < 0) {
+            return ret;
+        }
+
+        int64_t actual_size = bdrv_get_allocated_file_size(bs);
+
+        if (offset < actual_size) {

Again, offset is the guest disk size. With the file_size calculation above, what you probably want is "if (file_size < actual_size)".

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2141

Quoted 4 lines
block/qcow2.c +4 -0
+            int fd = qemu_open(bs->filename, O_WRONLY,
+                               O_APPEND);
+            if (fd >= 0) {
+                if (ftruncate(fd, offset) < 0) {

This should then be file_size instead of offset as well. Also, please don't use qemu_open() and ftruncate(), but bdrv_truncate() on bs->file.

Quoted 13 lines
block/qcow2.c +5 -0
+                    return -errno;
+                }
+                qemu_close(fd);
+            }
+        }
      }
 
      new_l1_size = size_to_l1(s, offset);
block/qcow2.h +0 -0
index 7d61e61..a9c8acb 100644

block/qcow2.h int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset, · line 523

Quoted 8 lines
block/qcow2.h +2 -0
@@ -516,6 +516,8 @@ int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
  /* qcow2-cluster.c functions */
  int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
                          bool exact_size);
+int qcow2_shrink_l1_and_l2_table(BlockDriverState *bs, uint64_t new_l1_size,
+                                 int new_l2_index, bool exact_size);
  int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
  void qcow2_l2_cache_reset(BlockDriverState *bs);
  int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);

Max

author
#3 lore raw

Re: [Qemu-devel] [PATCH v4 1/2] qcow2: Add qcow2_shrink_l1_and_l2_table for qcow2 shrinking

Quoted 39 lines · Max Reitz
Quoted 38 lines · Jun Li

This patch is the realization of new function qcow2_shrink_l1_and_l2_table. This function will shrink/discard l1 and l2 table when do qcow2 shrinking.

Signed-off-by Jun Li <[email protected]>

---
v4:
Add deal with COW clusters in l2 table. When using COW, some of (l2_entry >>
s->cluster_bits) will larger than s->refcount_table_size, so need to discard this l2_entry.
v3:
Fixed host cluster leak.

---
 block/qcow2-cluster.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c         |  40 +++++++++--
 block/qcow2.h         |   2 +
 3 files changed, 224 insertions(+), 4 deletions(-)
block/qcow2-cluster.c +8 -0
index f7dd8c0..0664b8a 100644
@@ -29,6 +29,9 @@
 #include "block/qcow2.h"
 #include "trace.h"
+static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
+                   uint64_t **l2_table);
+
 int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
                         bool exact_size)
 {

@@ -135,6 +138,189 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
     return ret;
 }
+int qcow2_shrink_l1_and_l2_table(BlockDriverState *bs, uint64_t new_l1_size,
+                                 int new_l2_index, bool exact_size)
+{
+    BDRVQcowState *s = bs->opaque;
+    int new_l1_size2, ret, i;
Quoted 8 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 151

Quoted 6 lines
block/qcow2-cluster.c +6 -0
+    uint64_t *new_l1_table;
+    int64_t new_l1_table_offset;
+    int64_t old_l1_table_offset, old_l1_size;
+    uint8_t data[12];
+
+    new_l1_size2 = sizeof(uint64_t) * new_l1_size;

new_l1_size is uint64_t, new_l1_size2 is int. Not a good idea.

This code is just copyed from qcow2_grow_l1_table. But sizeof(int) is 32 bits. So I think it's enough width here.

Quoted 5 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 153

Quoted 3 lines
block/qcow2-cluster.c +2 -0
+    new_l1_table = qemu_try_blockalign(bs->file,
+                                       align_offset(new_l1_size2, 512));

As long as with 512 you mean the sector size, s/512/BDRV_SECTOR_SIZE/.

Thank you.

Quoted 7 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 157

Quoted 5 lines
block/qcow2-cluster.c +4 -0
+    if (new_l1_table == NULL) {
+        return -ENOMEM;
+    }
+    memset(new_l1_table, 0, align_offset(new_l1_size2, 512));

Same here.

Ok, I will update it in v5.

Quoted 6 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 160

Quoted 4 lines
block/qcow2-cluster.c +3 -0
+
+    /* shrinking l1 table */
+    memcpy(new_l1_table, s->l1_table, new_l1_size2);

Please add an assert(new_l1_size <= s->l1_size) before this.

It's good.

Quoted 6 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 163

Quoted 4 lines
block/qcow2-cluster.c +3 -0
+
+    /* write new table (align to cluster) */
+    BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);

Well, it's not really growing, but fine.

I will aware of here in v5. Will add just like BLKDBG_L1_SHRINK_TABLE.

Quoted 18 lines
Quoted 18 lines
block/qcow2-cluster.c +17 -0
+    new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
+    if (new_l1_table_offset < 0) {
+        qemu_vfree(new_l1_table);
+        return new_l1_table_offset;
+    }
+
+    ret = qcow2_cache_flush(bs, s->refcount_block_cache);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* the L1 position has not yet been updated, so these clusters must
+     * indeed be completely free */
+    ret = qcow2_pre_write_overlap_check(bs, 0, new_l1_table_offset,
+                                        new_l1_size2);
+    if (ret < 0) {
+        goto fail;
Quoted 8 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 186

Quoted 6 lines
block/qcow2-cluster.c +6 -0
+    }
+
+    BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_WRITE_TABLE);
+
+    for (i = 0; i < new_l1_size; i++) {
+        new_l1_table[i] = cpu_to_be64(new_l1_table[i]);

You could have used cpu_to_be64s(&new_l1_table[i]) here.

ok~ But above is correct too.

Quoted 13 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 196

Quoted 11 lines
block/qcow2-cluster.c +10 -0
+    }
+
+    ret = bdrv_pwrite_sync(bs->file, new_l1_table_offset,
+                           new_l1_table, new_l1_size2);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    for (i = 0; i < new_l1_size; i++) {
+        new_l1_table[i] = be64_to_cpu(new_l1_table[i]);

Again, cpu_to_be64s() is an alternative.

ok.

Quoted 21 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 202

Quoted 7 lines
block/qcow2-cluster.c +6 -0
+    }
+
+    /* set new table */
+    BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ACTIVATE_TABLE);
+    cpu_to_be32w((uint32_t *)data, new_l1_size);
+    stq_be_p(data + 4, new_l1_table_offset);

Why not cpu_to_be64w()? I find it easier to read and it definitely fits in better.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 211

Quoted 9 lines
block/qcow2-cluster.c +9 -0
+    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_size),
+                           data, sizeof(data));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    old_l1_table_offset = s->l1_table_offset;
+    s->l1_table_offset = new_l1_table_offset;
+    uint64_t *old_l1_table = s->l1_table;
Quoted 1 line

Declarations have to be at the start of the block.

ok~

Quoted 9 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 216

Quoted 6 lines
block/qcow2-cluster.c +5 -0
+    s->l1_table = new_l1_table;
+    old_l1_size = s->l1_size;
+    s->l1_size = new_l1_size;
+
+    int num = old_l1_size - s->l1_size;

Same here.

oh~ :)

Quoted 19 lines
Quoted 19 lines
block/qcow2-cluster.c +19 -0
+
+    while (num >= 0) {
+        uint64_t l2_offset;
+        int ret;
+        uint64_t *l2_table, l2_entry;
+        int last_free_cluster = 0;
+
+        l2_offset = old_l1_table[s->l1_size + num - 1] & L1E_OFFSET_MASK;
+        if (l2_offset == 0) {
+            goto retry;
+        }
+
+        if (num == 0) {
+            if (new_l2_index == 0) {
+                goto retry;
+            }
+            last_free_cluster = new_l2_index + 1;
+        }
+
Quoted 8 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 240

Quoted 5 lines
block/qcow2-cluster.c +5 -0
+        /* load l2_table into cache */
+        ret = l2_load(bs, l2_offset, &l2_table);
+
+        if (ret < 0) {
+            goto fail;

Too bad we're freeing new_l1_table in fail. We should be freeing old_l1_table.

ok~

Quoted 9 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 244

Quoted 5 lines
block/qcow2-cluster.c +4 -0
+        }
+
+        for (i = s->l2_size; i > 0; i--) {
+            l2_entry = be64_to_cpu(l2_table[i - 1]);

Okay... I'd probably have gone with for (i = s->l2_size - 1; i >= 0; i--) and then l2_entry = be64_to_cpu(l2_table[i]).

Quoted 6 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 247

Quoted 3 lines
block/qcow2-cluster.c +3 -0
+
+            /* Deal with COW clusters in l2 table */
+            if ((i <= last_free_cluster)) {

I'd like some comments on what last_free_cluster is. Also, the inner parentheses should be removed.

last_free_cluster: When shrinking less than a l2 table, so we just need to discard some items of l2 table. "last_free_cluster" show that from this item of l2 table to the end will be discarded.

Quoted 9 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 249

Quoted 3 lines
block/qcow2-cluster.c +2 -0
+                if ((l2_entry >> s->cluster_bits) >
+                 s->refcount_table_size) {

Misalignment.

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 250

Quoted 1 line
block/qcow2-cluster.c +1 -0
+                    goto lable1;

It's "label", and "label1" is not a very helpful name.

Ok~

Quoted 32 lines
Quoted 32 lines
block/qcow2-cluster.c +31 -0
+                }
+                continue;
+            }
+
+            switch (qcow2_get_cluster_type(l2_entry)) {
+            case QCOW2_CLUSTER_UNALLOCATED:
+                if (!bs->backing_hd) {
+                    continue;
+                }
+                break;
+
+            case QCOW2_CLUSTER_ZERO:
+                continue;
+
+            case QCOW2_CLUSTER_NORMAL:
+            case QCOW2_CLUSTER_COMPRESSED:
+                break;
+
+            default:
+                abort();
+            }
+
+        lable1:
+            qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
+
+            uint64_t data64;
+            if (s->qcow_version >= 3) {
+                l2_table[i - 1] = cpu_to_be64(QCOW_OFLAG_ZERO);
+                data64 = cpu_to_be64(QCOW_OFLAG_ZERO);
+            } else {
+                l2_table[i - 1] = cpu_to_be64(0);
Quoted 8 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 287

Quoted 6 lines
block/qcow2-cluster.c +6 -0
+                data64 = cpu_to_be64(0);
+            }
+
+            /* before discard the specify item of l2_entry,
+             * set this entry with 0.
+             */

Why?

Just want to write it back to disk. But just go through the cache maybe is ok.

Quoted 6 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 290

Quoted 4 lines
block/qcow2-cluster.c +3 -0
+            ret = bdrv_pwrite_sync(bs->file,
+                                   l2_offset + (i-1) * sizeof(uint64_t),
+                                   &data64, sizeof(data64));

Why aren't you just going through the cache?

Ok~ yes, this is ok.

Quoted 9 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 297

Quoted 8 lines
block/qcow2-cluster.c +7 -0
+            if (ret < 0) {
+                goto fail;
+            }
+
+            /* Then decrease the refcount */
+            qcow2_free_any_clusters(bs, l2_entry, 1, QCOW2_DISCARD_MAX);
+        }
Quoted 6 lines

So I think what this code tried to achieve was freeing all data clusters referenced from L2 tables which are about to be dropped. Since this will change the data in these clusters anyway (and these clusters are not guest-accessible any more, so we don't have to care), just mark all of these L2 entries unallocated, don't bother with making them zero clusters.

I don't think it's a good idea.
e.g:
1, do shrinking image.
2. do grow this image.

As above show, do shrink in step1, then do grow in step2. If we do not set
the dropped item of l2 with 0, after step2 it's not so safety when execute "qemu-img check $IMG".

So I think we should deal with all the related tables(l1 talbe, l2 table, refcount table and refcount block table).

Quoted 26 lines

Simply marking the image dirty would be much easier, however. The problem is that this doesn't work for compat=0.10. I'd be fine (and by that, I mean I'd like it) with supporting shrinking only for compat=1.1 images.

Quoted 22 lines
block/qcow2-cluster.c +22 -0
+
+        ret = qcow2_cache_put(bs, s->l2_table_cache, (void **) &l2_table);
+        if (ret < 0) {
+            goto fail;
+        }
+        if (last_free_cluster == 0) {
+            qemu_vfree(l2_table);
+            qcow2_free_clusters(bs, l2_offset, s->cluster_size - 1,
+                                QCOW2_DISCARD_OTHER);
+        }
+    retry:
+        num--;
+    }
+
+    qemu_vfree(old_l1_table);
+
+    qcow2_free_clusters(bs, old_l1_table_offset, old_l1_size * sizeof(uint64_t),
+                        QCOW2_DISCARD_OTHER);
+    return 0;
+ fail:
+    qemu_vfree(new_l1_table);
+    qcow2_free_clusters(bs, new_l1_table_offset, new_l1_size2,
Quoted 8 lines

block/qcow2-cluster.c int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, · line 323

Quoted 4 lines
block/qcow2-cluster.c +4 -0
+                        QCOW2_DISCARD_OTHER);
+    return ret;
+}
+

So, all in all, I'd strongly propose just not supporting shrinking for compat=0.10; and for compat=1.1, we can simply mark the image dirty and then shrink the L1 table.

Does you mean do not consider drop some itmes of l2 table when it's necessary?

Quoted 16 lines
Quoted 9 lines
block/qcow2-cluster.c +0 -0
 /*
  * l2_load
  *
block/qcow2.c +0 -0
index fb28493..bed1558 100644

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2100

Quoted 5 lines
block/qcow2.c +1 -1
@@ -2097,7 +2097,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
     int64_t new_l1_size;
     int ret;
-    if (offset & 511) {
+    if (offset <= 0 || offset & 511) {
         error_report("The new size must be a multiple of 512");

This error message is inappropriate for offset <= 0.

ok~ I will update it in v5.

Quoted 4 lines

I think an assert(offset > 0) should be fine, too. bdrv_truncate() may not be called with a negative offset, as far as I know.

Quoted 13 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2112

Quoted 11 lines
block/qcow2.c +2 -3
         return -EINVAL;
     }

@@ -2108,10 +2108,42 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     }
-    /* shrinking is currently not supported */
+    /* shrinking image */
     if (offset < bs->total_sectors * 512) {
-        error_report("qcow2 doesn't support shrinking images yet");
-        return -ENOTSUP;
+

Spurious empty line?

Ok, checking...

Quoted 10 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2118

Quoted 6 lines
block/qcow2.c +5 -0
+        /* As l1 table, l2 table, refcount table, refcount block table
+         * and file header of the qcow2 image need to use some clusters,
+         * so should subtract these metadata from offset.
+         */
+        int64_t nb_l1 = (s->l1_size >> 3) >> s->cluster_bits;

s->l1_size is the number of entries in the L1 table. s->cluster_bits can be used to convert a number of bytes to a number of clusters. I guess what you want is (s->l1_size << 3) >> s->cluster_bits. But since s->l1_size << 3 does

                      ^^^

yes, you are right. Sorry~

Quoted 8 lines

not have to a multiple of the cluster size, I think DIV_ROUND_UP(s->l1_size
* sizeof(uint64_t), s->cluster_size) may be best (or DIV_ROUND_UP((uint64_t)s->l1_size * sizeof(uint64_t), s->cluster_size), since s->l1_size is only an int).

Quoted 5 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2119

Quoted 1 line
block/qcow2.c +1 -0
+        int64_t nb_l2 = offset / (s->l2_size << s->cluster_bits);

This should be DIV_ROUND_UP(offset, s->l2_size << s->cluster_bits). Maybe even DIV_ROUND_UP(offset, (uint64_t)s->l2_size << s->cluster_bits) since s->l2_size is only an int.

ok~ I will update it.

Quoted 9 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2123

Quoted 5 lines
block/qcow2.c +4 -0
+        int64_t nb_refcount_block_table = offset / ((s->cluster_size >>
+                                          REFCOUNT_SHIFT) << s->cluster_bits);
+        int64_t nb_refcount_table = (nb_refcount_block_table << 3) >>
+                                     s->cluster_bits;

The number of refblocks and the reftable size required does not depend on the offset which is the guest-visible image size. Instead, it depends on the host image size. Since the refblock count and the reftable size directly

yes, here is just consider the MAX scenario when "host image size" is equal to "guest-visible image size".

Quoted 7 lines

influences that host image size, this is not a trivial matter. See qcow2_create2() in the "if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC)" block for an example. Note that that function is not necessarily correct (a truly correct function for this should not be using floating point calculation, at the very least), but it was enough for that case. In this case however, we should do it right if we want to shrink the refcount table and reduce the number of refcount blocks.

I will update them just like followings:

        int64_t nb_l1 = ((s->l1_size << 3) >> s->cluster_bits) + 1;
        int64_t nb_l2 = (offset / (s->l2_size << s->cluster_bits)) + 1;
        int64_t nb_refcount_block_table = (offset / ((s->cluster_size >>
                                          REFCOUNT_SHIFT) << s->cluster_bits)) + 1;
        int64_t nb_refcount_table = ((nb_refcount_block_table << 3) >>
                                     s->cluster_bits) + 1;
        int64_t total_nb = nb_l2 + 2 * nb_l1 + nb_refcount_block_table +
                           nb_refcount_table + 1;

As above show, total_nb will use double of nb_l1 as we will reallocate clusters for l1 table in func qcow2_shrink_l1_and_l2_table.

Quoted 4 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2124

Quoted 2 lines
block/qcow2.c +1 -0
+        int64_t tatol_nb = nb_l2 + nb_l1 + nb_refcount_block_table +

Should be "total_nb".

Yes, :)

Quoted 7 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2126

Quoted 3 lines
block/qcow2.c +2 -0
+                           nb_refcount_table + 1;
+        int64_t offset_for_shrink = offset - (tatol_nb << s->cluster_bits);

offset is not the host image size but the guest disk size. It should be "file_size = offset + (total_nb << s->cluster_bits)" or something like that.

Quoted 7 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2127

Quoted 1 line
block/qcow2.c +1 -0
+        int new_l2_index = offset_to_l2_index(s, offset_for_shrink);

This would then be s/offset_for_shrink/offset/. Well, at least it should be. It would not be, because this new_l2_index is something strange, probably it's supposed to be the number of L2 indices to be culled. It shouldn't be. I think you want to pass the number of L2 indices to keep to qcow2_shrink_l1_and_l2_table(), that is, offset_to_l2_index(s, offset).

Here we just want to drop more clusters as meta data will use several clusters.
Here we just want to know how many items of l2 needed to drop. And we will use new_l2_index for last_free_cluster.

Quoted 5 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2129

Quoted 3 lines
block/qcow2.c +2 -0
+
+        new_l1_size = size_to_l1(s, offset_for_shrink);

s/offset_for_shrink/offset/.

Here, we just want to drop more clusters. So using offset_for_shrink not offset.

Quoted 12 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2137

Quoted 9 lines
block/qcow2.c +8 -0
+        ret = qcow2_shrink_l1_and_l2_table(bs, new_l1_size, new_l2_index, true);
+        if (ret < 0) {
+            return ret;
+        }
+
+        int64_t actual_size = bdrv_get_allocated_file_size(bs);
+
+        if (offset < actual_size) {

Again, offset is the guest disk size. With the file_size calculation above, what you probably want is "if (file_size < actual_size)".

when "file_size < actual_size"， we do not need to ftruncate this image on host.

Quoted 8 lines

block/qcow2.c static int qcow2_truncate(BlockDriverState *bs, int64_t offset) · line 2141

Quoted 5 lines
block/qcow2.c +4 -0
+            int fd = qemu_open(bs->filename, O_WRONLY,
+                               O_APPEND);
+            if (fd >= 0) {
+                if (ftruncate(fd, offset) < 0) {

This should then be file_size instead of offset as well. Also, please don't use qemu_open() and ftruncate(), but bdrv_truncate() on bs->file.

Ok, got it.

Quoted 13 lines
Quoted 13 lines
block/qcow2.c +5 -0
+                    return -errno;
+                }
+                qemu_close(fd);
+            }
+        }
     }
     new_l1_size = size_to_l1(s, offset);
block/qcow2.h +0 -0
index 7d61e61..a9c8acb 100644
@@ -516,6 +516,8 @@ int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
Quoted 9 lines

block/qcow2.h int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset, · line 523

Quoted 8 lines
block/qcow2.h +2 -0
 /* qcow2-cluster.c functions */
 int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
                         bool exact_size);
+int qcow2_shrink_l1_and_l2_table(BlockDriverState *bs, uint64_t new_l1_size,
+                                 int new_l2_index, bool exact_size);
 int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
 void qcow2_l2_cache_reset(BlockDriverState *bs);
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);

Hi max,

Thanks for your reviewer, I will submit a new version for this patch later.

In v5:
Will free some clusters firstly, then allocate clusters for l1 table in func qcow2_shrink_l1_and_l2_table.

Best Regards,
Jun Li

author
#4 lore raw

PATCH v4 2/2 qcow2: add update refcount table realization for update_refcount

In reply to Jun Li

When every item of refcount block is NULL, free refcount block and reset the corresponding item of refcount table with NULL. At the same time, put this cluster to s->free_cluster_index.

Signed-off-by Jun Li <[email protected]>

---
v4:
Do not change anything for this commit id.
v3:
Add handle self-describing refcount blocks.

---
 block/qcow2-refcount.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)
block/qcow2-refcount.c +45 -1
index 2bcaaf9..b603d4e 100644
@@ -28,6 +28,7 @@
 #include "qemu/range.h"
 
 static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size);
+static int write_reftable_entry(BlockDriverState *bs, int rt_index);
 static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
                             int64_t offset, int64_t length,
                             int addend, enum qcow2_discard_type type);

@@ -599,6 +600,49 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
         if (refcount == 0 && s->discard_passthrough[type]) {
             update_refcount_discard(bs, cluster_offset, s->cluster_size);
         }
+
+        unsigned int refcount_table_index;
+        refcount_table_index = cluster_index >> (s->cluster_bits -
+                               REFCOUNT_SHIFT);
+
+        uint64_t refcount_block_offset =
+            s->refcount_table[refcount_table_index] & REFT_OFFSET_MASK;
+
+        int64_t self_block_index = refcount_block_offset >> s->cluster_bits;
+        int self_block_index2 = (refcount_block_offset >> s->cluster_bits) &
+            ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);
+
+        /* When refcount block is NULL, update refcount table */
+        if (!be16_to_cpu(refcount_block[0]) || self_block_index2 == 0) {
+            int k;
+            int refcount_block_entries = s->cluster_size /
+                                         sizeof(refcount_block[0]);
+            for (k = 0; k < refcount_block_entries; k++) {
+                if (k == self_block_index2) {
+                    k++;
+                }
+
+                if (be16_to_cpu(refcount_block[k])) {
+                    break;
+                }
+            }
+
+            if (k == refcount_block_entries) {
+                if (self_block_index < s->free_cluster_index) {
+                    s->free_cluster_index = self_block_index;
+                }
+
+                /* update refcount table */
+                s->refcount_table[refcount_table_index] = 0;
+                ret = write_reftable_entry(bs, refcount_table_index);
+                if (ret < 0) {
+                    fprintf(stderr, "Could not update refcount table: %s\n",
+                            strerror(-ret));
+                    goto fail;
+                }
+
+            }
+        }
     }
 
     ret = 0;

@@ -1184,7 +1228,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
 
         case QCOW2_CLUSTER_NORMAL:
         {
-            uint64_t offset = l2_entry & L2E_OFFSET_MASK;
+            uint64_t offset = (uint64_t)(l2_entry & L2E_OFFSET_MASK);
 
             if (flags & CHECK_FRAG_INFO) {
                 res->bfi.allocated_clusters++;
Signature · 1 line
1.9.3