[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to u
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount. |
Date: |
Thu, 06 Nov 2008 19:11:58 +0100 |
User-agent: |
Thunderbird 2.0.0.17 (X11/20080922) |
Laurent Vivier schrieb:
> pièce jointe document texte brut
> (0002-Allow-update_cluster_refcount-to-update-several-cl.patch)
> To improve performance when the qcow2 file is empty, this patch
> allows update_cluster_refcount() to update refcount of
> several clusters.
>
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
> block-qcow2.c | 107
> ++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 68 insertions(+), 39 deletions(-)
>
> Index: qemu/block-qcow2.c
> ===================================================================
> --- qemu.orig/block-qcow2.c 2008-11-06 16:40:44.000000000 +0100
> +++ qemu/block-qcow2.c 2008-11-06 16:40:45.000000000 +0100
> @@ -159,6 +159,7 @@ static void refcount_close(BlockDriverSt
> static int get_refcount(BlockDriverState *bs, int64_t cluster_index);
> static int update_cluster_refcount(BlockDriverState *bs,
> int64_t cluster_index,
> + int nb_clusters,
> int addend);
> static void update_refcount(BlockDriverState *bs,
> int64_t offset, int64_t length,
> @@ -1711,7 +1712,7 @@ static int update_snapshot_refcount(Bloc
> refcount = 2;
> } else {
> if (addend != 0) {
> - refcount = update_cluster_refcount(bs, offset >>
> s->cluster_bits, addend);
> + refcount = update_cluster_refcount(bs, offset >>
> s->cluster_bits, 1, addend);
> } else {
> refcount = get_refcount(bs, offset >>
> s->cluster_bits);
> }
> @@ -1733,7 +1734,7 @@ static int update_snapshot_refcount(Bloc
> }
>
> if (addend != 0) {
> - refcount = update_cluster_refcount(bs, l2_offset >>
> s->cluster_bits, addend);
> + refcount = update_cluster_refcount(bs, l2_offset >>
> s->cluster_bits, 1, addend);
> } else {
> refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
> }
> @@ -2292,14 +2293,14 @@ static int64_t alloc_bytes(BlockDriverSt
> if (free_in_cluster == 0)
> s->free_byte_offset = 0;
> if ((offset & (s->cluster_size - 1)) != 0)
> - update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
> + update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
> } else {
> offset = alloc_clusters(bs, s->cluster_size);
> cluster_offset = s->free_byte_offset & ~(s->cluster_size - 1);
> if ((cluster_offset + s->cluster_size) == offset) {
> /* we are lucky: contiguous data */
> offset = s->free_byte_offset;
> - update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
> + update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
> s->free_byte_offset += size;
> } else {
> s->free_byte_offset = offset;
> @@ -2389,46 +2390,77 @@ static int grow_refcount_table(BlockDriv
> /* XXX: cache several refcount block clusters ? */
> static int update_cluster_refcount(BlockDriverState *bs,
> int64_t cluster_index,
> + int nb_clusters,
> int addend)
> {
> BDRVQcowState *s = bs->opaque;
> int64_t refcount_block_offset;
> - int ret, refcount_table_index, block_index, refcount;
> + int ret, refcount_table_index, refcount_table_last_index, block_index,
> refcount;
> + int nb_block_index;
> + int refcount_cache_size;
>
> - refcount_table_index = cluster_index >> (s->cluster_bits -
> REFCOUNT_SHIFT);
> - if (refcount_table_index >= s->refcount_table_size) {
> + if (nb_clusters == 0)
> + return 0;
> +
> + refcount_table_last_index = (cluster_index + nb_clusters - 1) >>
> + (s->cluster_bits - REFCOUNT_SHIFT);
> +
> + /* grow the refcount table if needed */
> +
> + if (refcount_table_last_index >= s->refcount_table_size) {
> if (addend < 0)
> return -EINVAL;
> - ret = grow_refcount_table(bs, refcount_table_index + 1);
> + ret = grow_refcount_table(bs, refcount_table_last_index + 1);
> if (ret < 0)
> return ret;
> }
> - refcount_block_offset = s->refcount_table[refcount_table_index];
> - if (!refcount_block_offset) {
> - if (addend < 0)
> - return -EINVAL;
> - refcount_block_offset = alloc_refcount_block(bs,
> refcount_table_index);
> - if (refcount_block_offset < 0)
> - return -EINVAL;
> - } else {
> - if (load_refcount_block(bs, refcount_block_offset) < 0)
> +
> + while (nb_clusters) {
> + refcount_table_index = cluster_index >>
> + (s->cluster_bits - REFCOUNT_SHIFT);
> + refcount_block_offset = s->refcount_table[refcount_table_index];
I guess (comment? ;-)) this outer loop is for handling requests which
span multiple refcount blocks? If so, am I missing something or is
refcount_block_offset the same for each iteration because cluster_index
never changes?
> +
> + if (!refcount_block_offset) {
> + if (addend < 0)
> + return -EINVAL;
> + refcount_block_offset = alloc_refcount_block(bs,
> refcount_table_index);
> + if (refcount_block_offset < 0)
> + return -EINVAL;
> + } else {
> + if (load_refcount_block(bs, refcount_block_offset) < 0)
> + return -EIO;
> + }
> +
> + /* we can update the count and save it */
> +
> + refcount_cache_size = 1 << (s->cluster_bits - REFCOUNT_SHIFT);
> + nb_block_index = 0;
I have to admit that nb_block_index is a name where I can't image what
the variable might be for. Seems to be a counter for the changed
refcounts in the current refcount block, and block_index seems to be the
first index to be touched in this block. What about first_index and
cur_refcount or something like that? (I don't like these suggestions too
much, either. Maybe someone has better names.)
> + block_index = cluster_index & (refcount_cache_size - 1);
> + refcount = 0;
> + while (nb_clusters &&
> + block_index + nb_block_index < refcount_cache_size) {
> +
> + refcount = be16_to_cpu(
> + s->refcount_block_cache[block_index +
> nb_block_index]);
> + refcount += addend;
> + if (refcount < 0 || refcount > 0xffff)
> + return -EINVAL;
Here we possibly abort in the middle of the operation. If it fails
somewhere in the fifth refcount block, what happens with the four
already updated blocks?
> + if (refcount == 0 &&
> + cluster_index + nb_block_index < s->free_cluster_index) {
> + s->free_cluster_index = cluster_index + nb_block_index;
> + }
> + s->refcount_block_cache[block_index + nb_block_index] =
> +
> cpu_to_be16(refcount);
> + nb_block_index++;
> + nb_clusters--;
> + }
> + if (bdrv_pwrite(s->hd,
> + refcount_block_offset + (block_index <<
> REFCOUNT_SHIFT),
> + s->refcount_block_cache + block_index,
> + nb_block_index * sizeof(uint16_t)) !=
> + nb_block_index * sizeof(uint16_t))
> return -EIO;
Same here.
> }
> - /* we can update the count and save it */
> - block_index = cluster_index &
> - ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);
> - refcount = be16_to_cpu(s->refcount_block_cache[block_index]);
> - refcount += addend;
> - if (refcount < 0 || refcount > 0xffff)
> - return -EINVAL;
> - if (refcount == 0 && cluster_index < s->free_cluster_index) {
> - s->free_cluster_index = cluster_index;
> - }
> - s->refcount_block_cache[block_index] = cpu_to_be16(refcount);
> - if (bdrv_pwrite(s->hd,
> - refcount_block_offset + (block_index << REFCOUNT_SHIFT),
> - &s->refcount_block_cache[block_index], 2) != 2)
> - return -EIO;
> return refcount;
> }
>
> @@ -2437,7 +2469,7 @@ static void update_refcount(BlockDriverS
> int addend)
> {
> BDRVQcowState *s = bs->opaque;
> - int64_t start, last, cluster_offset;
> + int64_t start, last;
>
> #ifdef DEBUG_ALLOC2
> printf("update_refcount: offset=%lld size=%lld addend=%d\n",
> @@ -2445,12 +2477,9 @@ static void update_refcount(BlockDriverS
> #endif
> if (length <= 0)
> return;
> - start = offset & ~(s->cluster_size - 1);
> - last = (offset + length - 1) & ~(s->cluster_size - 1);
> - for(cluster_offset = start; cluster_offset <= last;
> - cluster_offset += s->cluster_size) {
> - update_cluster_refcount(bs, cluster_offset >> s->cluster_bits,
> addend);
> - }
> + start = offset >> s->cluster_bits;
> + last = (offset + length) >> s->cluster_bits;
Off by one for length % cluster_size == 0?
> + update_cluster_refcount(bs, start, last - start + 1, addend);
> }
>
> #ifdef DEBUG_ALLOC
>
Kevin