| Message ID | 1287417233-23688-1-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 18.10.2010 17:53, schrieb Stefan Hajnoczi: > The L1 table grow operation includes a size calculation that bumps up > the new L1 table size in order to anticipate the size needs of vmstate > data. This helps reduce the number of times that the L1 table has to be > grown when vmstate data is appended. > > This size overhead is not necessary during image creation, > bdrv_truncate(), or snapshot goto operations. In fact, existing > qemu-iotests that exercise table growth are no longer able to trigger it > because image creation preallocates an L1 table that is too large after > changes to qcow_create2(). > > This patch keeps the size calculation but also adds exact growth for > callers that do not want to inflate the L1 table size unnecessarily. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > --- > block/qcow2-cluster.c | 25 ++++++++++++++++--------- > block/qcow2-snapshot.c | 2 +- > block/qcow2.c | 2 +- > block/qcow2.h | 2 +- > 4 files changed, 19 insertions(+), 12 deletions(-) > > Hi Kevin, > This patch fixes the qcow_create2() issue seen in qemu-iotests 026 with your > kevin.git/block branch. The issue was that the L1 table size of new images is > inflated by qcow2_grow_l1_table(). This caused the differences in the test, > e.g. L1 table grow tests no longer worked because they couldn't force the table > to grow (it was already more than large enough). > > If we use exact L1 growth in bdrv_truncate() then less image space is wasted > and the test passes again without changes to 026.out. > > I think this patch is the way to go, not just to satisfy the test, but also > because we don't need to overallocate L1 tables to start with. Good that you took a look at it, I hadn't even thought of changing the qcow2 code. I agree that this makes sense even independent of qemu-iotests. The patch looks good to me, too. Kevin
Am 18.10.2010 17:53, schrieb Stefan Hajnoczi: > The L1 table grow operation includes a size calculation that bumps up > the new L1 table size in order to anticipate the size needs of vmstate > data. This helps reduce the number of times that the L1 table has to be > grown when vmstate data is appended. > > This size overhead is not necessary during image creation, > bdrv_truncate(), or snapshot goto operations. In fact, existing > qemu-iotests that exercise table growth are no longer able to trigger it > because image creation preallocates an L1 table that is too large after > changes to qcow_create2(). > > This patch keeps the size calculation but also adds exact growth for > callers that do not want to inflate the L1 table size unnecessarily. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> Thanks, applied to the block branch. Kevin
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index fb4224a..4f7dc59 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -28,7 +28,7 @@ #include "block_int.h" #include "block/qcow2.h" -int qcow2_grow_l1_table(BlockDriverState *bs, int min_size) +int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size) { BDRVQcowState *s = bs->opaque; int new_l1_size, new_l1_size2, ret, i; @@ -36,15 +36,22 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size) int64_t new_l1_table_offset; uint8_t data[12]; - new_l1_size = s->l1_size; - if (min_size <= new_l1_size) + if (min_size <= s->l1_size) return 0; - if (new_l1_size == 0) { - new_l1_size = 1; - } - while (min_size > new_l1_size) { - new_l1_size = (new_l1_size * 3 + 1) / 2; + + if (exact_size) { + new_l1_size = min_size; + } else { + /* Bump size up to reduce the number of times we have to grow */ + new_l1_size = s->l1_size; + if (new_l1_size == 0) { + new_l1_size = 1; + } + while (min_size > new_l1_size) { + new_l1_size = (new_l1_size * 3 + 1) / 2; + } } + #ifdef DEBUG_ALLOC2 printf("grow l1_table from %d to %d\n", s->l1_size, new_l1_size); #endif @@ -550,7 +557,7 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset, l1_index = offset >> (s->l2_bits + s->cluster_bits); if (l1_index >= s->l1_size) { - ret = qcow2_grow_l1_table(bs, l1_index + 1); + ret = qcow2_grow_l1_table(bs, l1_index + 1, false); if (ret < 0) { return ret; } diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 5539510..aacf357 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -327,7 +327,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, -1) < 0) goto fail; - if (qcow2_grow_l1_table(bs, sn->l1_size) < 0) + if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0) goto fail; s->l1_size = sn->l1_size; diff --git a/block/qcow2.c b/block/qcow2.c index d5b7b1a..b816d87 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1061,7 +1061,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset) } new_l1_size = size_to_l1(s, offset); - ret = qcow2_grow_l1_table(bs, new_l1_size); + ret = qcow2_grow_l1_table(bs, new_l1_size, true); if (ret < 0) { return ret; } diff --git a/block/qcow2.h b/block/qcow2.h index d1275cd..2d22e5e 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -188,7 +188,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res); /* qcow2-cluster.c functions */ -int qcow2_grow_l1_table(BlockDriverState *bs, int min_size); +int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size); void qcow2_l2_cache_reset(BlockDriverState *bs); int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset); void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
The L1 table grow operation includes a size calculation that bumps up the new L1 table size in order to anticipate the size needs of vmstate data. This helps reduce the number of times that the L1 table has to be grown when vmstate data is appended. This size overhead is not necessary during image creation, bdrv_truncate(), or snapshot goto operations. In fact, existing qemu-iotests that exercise table growth are no longer able to trigger it because image creation preallocates an L1 table that is too large after changes to qcow_create2(). This patch keeps the size calculation but also adds exact growth for callers that do not want to inflate the L1 table size unnecessarily. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- block/qcow2-cluster.c | 25 ++++++++++++++++--------- block/qcow2-snapshot.c | 2 +- block/qcow2.c | 2 +- block/qcow2.h | 2 +- 4 files changed, 19 insertions(+), 12 deletions(-) Hi Kevin, This patch fixes the qcow_create2() issue seen in qemu-iotests 026 with your kevin.git/block branch. The issue was that the L1 table size of new images is inflated by qcow2_grow_l1_table(). This caused the differences in the test, e.g. L1 table grow tests no longer worked because they couldn't force the table to grow (it was already more than large enough). If we use exact L1 growth in bdrv_truncate() then less image space is wasted and the test passes again without changes to 026.out. I think this patch is the way to go, not just to satisfy the test, but also because we don't need to overallocate L1 tables to start with.