Re: [Qemu-devel] [PATCH v5 08/26] qcow2: Refcount overflow and qcow2_alloc_bytes()

[Max Reitz <mreitz@redhat.com>]

On 2015-02-04 at 06:55, Kevin Wolf wrote:
> Am 15.12.2014 um 13:50 hat Max Reitz geschrieben:
>> qcow2_alloc_bytes() may reuse a cluster multiple times, in which case
>> the refcount is increased accordingly. However, if this would lead to an
>> overflow the function should instead just not reuse this cluster and
>> allocate a new one.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/qcow2-refcount.c | 31 ++++++++++++++++++++++++++++++-
>>   1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index db81647..fd28a13 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -780,9 +780,11 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
>>       BDRVQcowState *s = bs->opaque;
>>       int64_t offset, cluster_offset, new_cluster;
>>       int free_in_cluster, ret;
>> +    uint64_t refcount;
>>   
>>       BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_BYTES);
>>       assert(size > 0 && size <= s->cluster_size);
>> + redo:
>>       if (s->free_byte_offset == 0) {
>>           offset = qcow2_alloc_clusters(bs, s->cluster_size);
>>           if (offset < 0) {
>> @@ -790,12 +792,25 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
>>           }
>>           s->free_byte_offset = offset;
>>       }
>> - redo:
>> +
>>       free_in_cluster = s->cluster_size -
>>           offset_into_cluster(s, s->free_byte_offset);
>>       if (size <= free_in_cluster) {
>>           /* enough space in current cluster */
>>           offset = s->free_byte_offset;
>> +
>> +        if (offset_into_cluster(s, offset) != 0) {
>> +            /* We will have to increase the refcount of this cluster; if the
>> +             * maximum has been reached already, this cluster cannot be used */
>> +            ret = qcow2_get_refcount(bs, offset >> s->cluster_bits, &refcount);
>> +            if (ret < 0) {
>> +                return ret;
>> +            } else if (refcount == s->refcount_max) {
>> +                s->free_byte_offset = 0;
>> +                goto redo;
>> +            }
>> +        }
>> +
>>           s->free_byte_offset += size;
>>           free_in_cluster -= size;
>>           if (free_in_cluster == 0)
>> @@ -816,6 +831,20 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
>>           if ((cluster_offset + s->cluster_size) == new_cluster) {
>>               /* we are lucky: contiguous data */
>>               offset = s->free_byte_offset;
>> +
>> +            /* Same as above: In order to reuse the cluster, the refcount has to
>> +             * be increased; if that will not work, we are not so lucky after
>> +             * all */
>> +            ret = qcow2_get_refcount(bs, offset >> s->cluster_bits, &refcount);
>> +            if (ret < 0) {
>> +                qcow2_free_clusters(bs, new_cluster, s->cluster_size,
>> +                                    QCOW2_DISCARD_NEVER);
>> +                return ret;
>> +            } else if (refcount == s->refcount_max) {
>> +                s->free_byte_offset = offset;
> I think you mean 0. offset is already the old value.

Oh, right. Thanks for catching!

>> +                goto redo;
>> +            }
>> +
>>               ret = qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits,
>>                                                   1, false, QCOW2_DISCARD_NEVER);
>>               if (ret < 0) {
> I wonder if the code duplication is necessary. I was already thinking
> that there was some duplication when I reviewed the previous patch, but
> now it seems to become even more obvious that the three parts of this
> function are:
>
> 1. Allocate a new cluster
> 2. Allocate space in the already allocated cluster
> 3. Allocate a new cluster and space inside it, which is just 1. + 2.

Well, I can try rewriting this function, but I guess the diffcount will 
be even higher.

I think I'll try to write an independent patch which rewrites this 
function and then drop these two patches from this series.

Max