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

[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)

Paolo