Re: [Qemu-devel] [PATCH] qcow2: Simplify calculation for COW area at the end

[Kevin Wolf]

Am 12.06.2012 16:31, schrieb Paolo Bonzini:
> Il 12/06/2012 16:21, Kevin Wolf ha scritto:
>>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>>> index 9aee9fc..763b724 100644
>>>> --- a/block/qcow2-cluster.c
>>>> +++ b/block/qcow2-cluster.c
>>>> @@ -640,11 +640,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState 
>>>> *bs, QCowL2Meta *m)
>>>>      }
>>>>  
>>>>      if (m->nb_available & (s->cluster_sectors - 1)) {
>>>> -        uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 
>>>> 1);
>>>>          cow = true;
>>>>          qemu_co_mutex_unlock(&s->lock);
>>>> -        ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 
>>>> 9),
>>>> -                m->nb_available - end, s->cluster_sectors);
>>>> +        ret = copy_sectors(bs, start_sect, cluster_offset,
>>>> +                m->nb_available, s->cluster_sectors);
>>>
>>> Do you need to add end to s->cluster_sectors too, so that "start_sect +
>>> n_end" and "n_end - n_start" remain the same?
>>
>> You mean because n_end is now relative to start_sect instead of
>> start_sect + end, right?
> 
> Yes.  Or more simply, because I was expecting no other uses of
> start_sect, cluster_offset and n_start after reading your commit message. :)
> 
>> I thought about it and I find this code is a bit confusing, but I think
>> you're right that I need to replace n_end as well because it would be
>> wrong for an allocating request than spans multiple clusters. I think
>> this one should be right, would you agree?
>>
>> ret = copy_sectors(bs, start_sect, cluster_offset,
>>    m->nb_available, align_offset(m->nb_available, s->cluster_sectors));
> 
> The obvious expression would be
> 
>        s->cluster_sectors
>          + (m->nb_available & ~(uint64_t)(s->cluster_sectors - 1))
> 
> which is a bit different from align_offset.  If m->nb_available is
> already aligned, it returns the *next* aligned value rather than
> m->nb_available itself.
> 
> So the equivalent expression using align_offset would be this one:
> 
>        align_offset(m->nb_available+1, s->cluster_sectors)

Heh, yes, you're thinking about equivalence of the very formula (which
is what I would do in unknown code as well), whereas I think about a COW
operation that ranges from a given sector to the end of the same cluster
(and not the next one).

It's eventually the same, because this statement is only executed when
it's not aligned (i.e. the COW range isn't empty):

   if (m->nb_available & (s->cluster_sectors - 1)) {
       ...
   }

Kevin