Re: [Qemu-devel] [PATCH v5 2/3] qcow2: add update refcount table realization for update_refcount

[Max Reitz <mreitz@redhat.com>]

On 2014-10-26 at 16:20, Jun Li wrote:
> 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 <junmuzi@gmail.com>
> ---
> v5:
>    Just instead write_reftable_entry with bdrv_pwrite_sync. As write_reftable_entry has deleted from the source code.
>
> v4:
>    Do not change anything for this commit id.
>
> v3:
>    Add handle self-describing refcount blocks.
> ---
>   block/qcow2-refcount.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 48 insertions(+), 1 deletion(-)

Well, this patch will conflict with my "qcow2: Support refcount orders 
!= 4" series, but that's how it is.

> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 1477031..abb4f6d 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -596,6 +596,53 @@ 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);
>           }

We can skip the whole following block if refcount > 0, so we should do that.

> +
> +        unsigned int refcount_table_index;

Declarations only at the start of the block, please. Furthermore, this 
is the same as table_index.

> +        refcount_table_index = cluster_index >> s->refcount_block_bits;
> +
> +        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->refcount_block_bits) - 1);

You are assuming here that every refblock is described by itself. While 
that may be true for QEMU's current qcow2 driver, it's by no means 
defined that way.

> +
> +        /* When refcount block is NULL, update refcount table */
> +        if (!be16_to_cpu(refcount_block[0]) || self_block_index2 == 0) {

Is this check really necessary? I think we can just go into the for () 
loop. If the first entry is not 0, it will be exited immediately anyway.

> +            int k;
> +            int refcount_block_entries = s->cluster_size /
> +                                         sizeof(refcount_block[0]);

s->refcount_block_size is what you want (but we probably didn't have it 
in October yet).

> +            for (k = 0; k < refcount_block_entries; k++) {
> +                if (k == self_block_index2) {
> +                    k++;

This should be replaced by a "continue", otherwise the following array 
access will overflow if self_block_index2 == refcount_block_entries - 1;

> +                }
> +
> +                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;
> +                }

You should probably flush the refblock cache here. When the cluster 
which was occupied by the refblock is marked free, it may be used for 
different data, while the refblock remains cached. When the cache entry 
is removed later on, it will be written to disk and overwrite that cluster.

Also, you're again assuming that every refblock handles its own 
refcount, which is not necessarily true. The refcount may be handled by 
a different refblock in which case you have to decrement its refcount there.

> +
> +                /* update refcount table */
> +                s->refcount_table[refcount_table_index] = 0;
> +                uint64_t data64 = cpu_to_be64(0);
> +                ret = bdrv_pwrite_sync(bs->file,
> +                                       s->refcount_table_offset +
> +                                       refcount_table_index *
> +                                       sizeof(uint64_t),
> +                                       &data64, sizeof(data64));
> +                if (ret < 0) {
> +                    fprintf(stderr, "Could not update refcount table: %s\n",
> +                            strerror(-ret));
> +                    goto fail;
> +                }
> +

There's an empty line here which is probably unneeded. Also, you may 
want to do an overlap check before updating the reftable.

> +            }
> +        }
>       }
>   
>       ret = 0;
> @@ -1204,7 +1251,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);

Is that supposed to be in this patch? Also, what does it do? l2_entry is 
uint64_t anyway, so the result of l2_entry & L2E_OFFSET_MASK should 
already be of type uint64_t.

>               if (flags & CHECK_FRAG_INFO) {
>                   res->bfi.allocated_clusters++;

So, about this whole patch: I'm not sure whether we need it in this 
series but I remember you saying that it's actually worth it, so I'm 
trusting you.

Second, I somehow don't like iterating over the whole refblock for each 
refcount updated or at least for each refcount set to 0. There are 32768 
entries per refblock with 16 bit refcounts and 64 kB clusters. 
Therefore, when freeing all clusters referenced by a refblock (and if 
that refblock doesn't have any free entries yet), we will loop over 
16384 entries (in average) before noticing there are still entries with 
a non-zero refcount, and that we will do 32768 times (okay, 32767, 
because you're assuming a refblock always contains its own refcount). I 
think that a bit much. But I don't have any idea to fix this other than 
keeping the number of entries per refblock with refcount != 0 in a table 
in memory (which is probably not too bad, but not too nice either).

Max