Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount.

[Kevin Wolf]

Laurent Vivier schrieb:
>>> +        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?
> 
> Yes, you're right. I think we have at least to save refcount we have
> updated.
> 
>>> +            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.
> 
> I think we are not worst than the original behavior. I don't know how to
> manage this better.

Right, this hasn't been optimal before either. I noticed later that
update_refcount didn't even check the return value. So the difference is
that you abort while the old implementation tries the next block.

I'm unsure which behaviour is the better one. Actually, both don't feel
quite right. But being not worse in this aspect than the original still
makes this patch an improvement, so...

>>>      }
>>> -    /* 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?
> 
> Explain, please (but notice the "<=" in the "for (...)").

I didn't even look at the old code yesterday, but the difference is that
you dropped the - 1 for last.

Let's do it by example: Assume cluster_size = 0x1000 for simplicity and
we get offset = 0x1000 and length = 0x2000 (i.e. the last affected byte
would be 0x2fff). The old code correctly produces start = 0x1000 and
last = 0x2000 whereas you get start = 1 and last = 3.

Kevin