On Mon, Sep 01, 2014 at 06:52:48PM +0800, Jun Li wrote:

How does this patch handle self-describing refcount blocks?  I think
they will keep the refcount block alive forever because your code will
not decide to free them.

This patch should also discard the refcount block if we decide to free
it (in the same way that we discard at cluster_offset).

> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 43665b8..63f36e6 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -586,6 +586,37 @@ 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);
>          }
> +
> +        /* When refcount block is NULL, update refcount table */
> +        if (block_index == 0) {

What is the purpose of block_index == 0?

> +            int k = block_index;
> +            int refcount_block_entries = s->cluster_size / sizeof(uint16_t);
> +            for (k = 0; k < refcount_block_entries; k++) {
> +                if (refcount_block[k] != cpu_to_be16(0)) {
> +                    break;
> +                }
> +            }
> +
> +            if (k == refcount_block_entries) {
> +                qemu_vfree(refcount_block);

You can't do this, the buffer belongs to the refcount block cache.
Please look at the cache get/put as well as qcow2_cache_create/destroy.

> +                /* update refcount table */
> +                unsigned int refcount_table_index;
> +                uint64_t data64 = cpu_to_be64(0);
> +                refcount_table_index = cluster_index >> (s->cluster_bits -
> +                                       REFCOUNT_SHIFT);
> +                ret = bdrv_pwrite_sync(bs->file,
> +                                       s->refcount_table_offset +
> +                                       refcount_table_index *
> +                                       sizeof(uint64_t),
> +                                       &data64, sizeof(data64));
> +                if (ret < 0) {
> +                    goto fail;
> +                }

Plase use write_reftable_entry().