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

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

[Kevin Wolf]

copy_sectors() always uses the sum (cluster_offset + n_start) or
(start_sect + n_start), so if some value is added to both cluster_offset
and start_sect, and subtracted from n_start, it's cancelled out anyway.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

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);
         qemu_co_mutex_lock(&s->lock);
         if (ret < 0)
             goto err;
-- 
1.7.6.5

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

[Paolo Bonzini]

Il 12/06/2012 15:47, Kevin Wolf ha scritto:
> copy_sectors() always uses the sum (cluster_offset + n_start) or
> (start_sect + n_start), so if some value is added to both cluster_offset
> and start_sect, and subtracted from n_start, it's cancelled out anyway.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2-cluster.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> 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?

Paolo

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

[Kevin Wolf]

Am 12.06.2012 16:00, schrieb Paolo Bonzini:
> Il 12/06/2012 15:47, Kevin Wolf ha scritto:
>> copy_sectors() always uses the sum (cluster_offset + n_start) or
>> (start_sect + n_start), so if some value is added to both cluster_offset
>> and start_sect, and subtracted from n_start, it's cancelled out anyway.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block/qcow2-cluster.c |    5 ++---
>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> 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?

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 interesting question is why qemu-iotests doesn't catch it.

Kevin

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

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

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

[Paolo Bonzini]

Il 12/06/2012 16:37, Kevin Wolf ha scritto:
>> >        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)) {
>        ...
>    }

Indeed, your version is ok.

Paolo