Re: [Qemu-devel] [PATCH 00/26] My block patches queue for 1.1

Re: [Qemu-devel] [PATCH 00/26] My block patches queue for 1.1

[Paolo Bonzini]

On Thu, Apr 12, 2012 at 2:00 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Here is my block patches queue for 1.1.
>
> Patches 1-6 are cleanups to tools and aio.c that let filesystems
> use timers when running under qemu-io.
>
> Patches 7-12 are bugfixes from last week's series, patches 13-14
> are new.
>
> Patches 15-19 are fixes to qemu-io and qemu-iotests.
>
> Patches 20-22 are fixes to streaming.  Patch 22 modifies it to
> not stream unallocated areas of the base (see rationale in the
> commit message).  For this reason I'm including in this series
> also patch 23, which implements is_allocated for raw-posix.
>
> Finally, patches 24-26 are small changes to streaming that move
> some code outside block/stream.c for more general usage.

Kevin, what should I do about these patches?  It needs rebasing, but
there are several bugfixes in here, and there are more that I need to
submit. Some ideas about what I could do to help:

- split it in chunks, and see where we get by the time of 1.1 (risk of
not finding problems in late patches)

- find patches that are in RHEL, prod RHEL reviewers for a Reviewed-by.

...

Paolo

Re: [Qemu-devel] [PATCH 00/26] My block patches queue for 1.1

[Kevin Wolf]

Am 04.05.2012 18:20, schrieb Paolo Bonzini:
> On Thu, Apr 12, 2012 at 2:00 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Here is my block patches queue for 1.1.
>>
>> Patches 1-6 are cleanups to tools and aio.c that let filesystems
>> use timers when running under qemu-io.
>>
>> Patches 7-12 are bugfixes from last week's series, patches 13-14
>> are new.
>>
>> Patches 15-19 are fixes to qemu-io and qemu-iotests.
>>
>> Patches 20-22 are fixes to streaming.  Patch 22 modifies it to
>> not stream unallocated areas of the base (see rationale in the
>> commit message).  For this reason I'm including in this series
>> also patch 23, which implements is_allocated for raw-posix.
>>
>> Finally, patches 24-26 are small changes to streaming that move
>> some code outside block/stream.c for more general usage.
> 
> Kevin, what should I do about these patches?  It needs rebasing, but
> there are several bugfixes in here, and there are more that I need to
> submit. Some ideas about what I could do to help:
> 
> - split it in chunks, and see where we get by the time of 1.1 (risk of
> not finding problems in late patches)
> 
> - find patches that are in RHEL, prod RHEL reviewers for a Reviewed-by.

First of all, please make sure to have me in the CC list. I try to scan
qemu-devel regularly, but when I'm busy it can happen that I don't for
some days.

If the patches need rebasing, rebase and resend them. It would be good
if you can limit the list of patches required for 1.1, but if all of
them are true fixes, we can still get them merged. Splitting into chunks
isn't necessary, but it would be helpful if you could put them in the
order of their importance, so that applying the series partially still
makes sense if necessary. Review is always appreciated, but it doesn't
really affect what patches to send.

Kevin

Re: [Qemu-devel] [PATCH 17/26] qemu-io: fix the alloc command

[Kevin Wolf]

Am 12.04.2012 14:01, schrieb Paolo Bonzini:
> Because sector_num is not updated, the loop would either go on
> forever or return garbage.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-io.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-io.c b/qemu-io.c
> index 43643c8..27a0c3c 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -1560,7 +1560,7 @@ out:
>  
>  static int alloc_f(int argc, char **argv)
>  {
> -    int64_t offset;
> +    int64_t offset, sector_num;
>      int nb_sectors, remaining;
>      char s1[64];
>      int num, sum_alloc;
> @@ -1581,12 +1581,18 @@ static int alloc_f(int argc, char **argv)
>  
>      remaining = nb_sectors;
>      sum_alloc = 0;
> +    sector_num = offset >> 9;
>      while (remaining) {
> -        ret = bdrv_is_allocated(bs, offset >> 9, nb_sectors, &num);
> +        ret = bdrv_is_allocated(bs, sector_num, remaining, &num);
> +        sector_num += num;
>          remaining -= num;
>          if (ret) {
>              sum_alloc += num;
>          }
> +        if (num == 0) {
> +            nb_sectors -= remaining;
> +            remaining = 0;
> +        }
>      }
>  
>      cvtstr(offset, s1, sizeof(s1));

This doesn't provide the semantics I expected, i.e. the semantics of
bdrv_is_allocated, which is the number of contiguous clusters that are
allocated or unallocated. Instead you provide the number of all
allocated in the whole area even if there are some unallocated clusters
in the middle.

Do you think there's a use case for the sum of the whole area?

Kevin

Re: [Qemu-devel] [PATCH 17/26] qemu-io: fix the alloc command

[Paolo Bonzini]

Il 08/05/2012 14:57, Kevin Wolf ha scritto:
>> > 
>> > diff --git a/qemu-io.c b/qemu-io.c
>> > index 43643c8..27a0c3c 100644
>> > --- a/qemu-io.c
>> > +++ b/qemu-io.c
>> > @@ -1560,7 +1560,7 @@ out:
>> >  
>> >  static int alloc_f(int argc, char **argv)
>> >  {
>> > -    int64_t offset;
>> > +    int64_t offset, sector_num;
>> >      int nb_sectors, remaining;
>> >      char s1[64];
>> >      int num, sum_alloc;
>> > @@ -1581,12 +1581,18 @@ static int alloc_f(int argc, char **argv)
>> >  
>> >      remaining = nb_sectors;
>> >      sum_alloc = 0;
>> > +    sector_num = offset >> 9;
>> >      while (remaining) {
>> > -        ret = bdrv_is_allocated(bs, offset >> 9, nb_sectors, &num);
>> > +        ret = bdrv_is_allocated(bs, sector_num, remaining, &num);
>> > +        sector_num += num;
>> >          remaining -= num;
>> >          if (ret) {
>> >              sum_alloc += num;
>> >          }
>> > +        if (num == 0) {
>> > +            nb_sectors -= remaining;
>> > +            remaining = 0;
>> > +        }
>> >      }
>> >  
>> >      cvtstr(offset, s1, sizeof(s1));
> This doesn't provide the semantics I expected, i.e. the semantics of
> bdrv_is_allocated, which is the number of contiguous clusters that are
> allocated or unallocated. Instead you provide the number of all
> allocated in the whole area even if there are some unallocated clusters
> in the middle.

commit a7824a886ed50eb4fe3c6fcd6afd8814a6973583
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Mon Jul 20 16:48:43 2009 +0200

    qemu-io: Rework alloc command
    
    The alloc command in qemu-io is mostly useless currently. Instead of doing a
    single call to bdrv_is_allocated, we must call bdrv_is_allocated in a loop
    until we have found out for each requested sector if it is allocated or not
    (bdrv_is_allocated returns a number of sectors that are known to be in the same
    state as the first one, but it is not required to include all of them)
    
    This changes the output format of the alloc command so that a change to the
    expected output of qemu-iotests 019 is necessary once this is included.
    
    Signed-off-by: Kevin Wolf <kwolf@redhat.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

I guess this guy knows. :)

> Do you think there's a use case for the sum of the whole area?

I think for tests it's more useful, you don't want to depend on the implementation
of is_allocated.  That's what I can guess from the above commit message and from
the way 019 uses alloc.

Paolo

Re: [Qemu-devel] [PATCH 17/26] qemu-io: fix the alloc command

[Kevin Wolf]

Am 08.05.2012 15:06, schrieb Paolo Bonzini:
> Il 08/05/2012 14:57, Kevin Wolf ha scritto:
>>>>
>>>> diff --git a/qemu-io.c b/qemu-io.c
>>>> index 43643c8..27a0c3c 100644
>>>> --- a/qemu-io.c
>>>> +++ b/qemu-io.c
>>>> @@ -1560,7 +1560,7 @@ out:
>>>>  
>>>>  static int alloc_f(int argc, char **argv)
>>>>  {
>>>> -    int64_t offset;
>>>> +    int64_t offset, sector_num;
>>>>      int nb_sectors, remaining;
>>>>      char s1[64];
>>>>      int num, sum_alloc;
>>>> @@ -1581,12 +1581,18 @@ static int alloc_f(int argc, char **argv)
>>>>  
>>>>      remaining = nb_sectors;
>>>>      sum_alloc = 0;
>>>> +    sector_num = offset >> 9;
>>>>      while (remaining) {
>>>> -        ret = bdrv_is_allocated(bs, offset >> 9, nb_sectors, &num);
>>>> +        ret = bdrv_is_allocated(bs, sector_num, remaining, &num);
>>>> +        sector_num += num;
>>>>          remaining -= num;
>>>>          if (ret) {
>>>>              sum_alloc += num;
>>>>          }
>>>> +        if (num == 0) {
>>>> +            nb_sectors -= remaining;
>>>> +            remaining = 0;
>>>> +        }
>>>>      }
>>>>  
>>>>      cvtstr(offset, s1, sizeof(s1));
>> This doesn't provide the semantics I expected, i.e. the semantics of
>> bdrv_is_allocated, which is the number of contiguous clusters that are
>> allocated or unallocated. Instead you provide the number of all
>> allocated in the whole area even if there are some unallocated clusters
>> in the middle.
> 
> commit a7824a886ed50eb4fe3c6fcd6afd8814a6973583
> Author: Kevin Wolf <kwolf@redhat.com>
> Date:   Mon Jul 20 16:48:43 2009 +0200
> 
>     qemu-io: Rework alloc command
>     
>     The alloc command in qemu-io is mostly useless currently. Instead of doing a
>     single call to bdrv_is_allocated, we must call bdrv_is_allocated in a loop
>     until we have found out for each requested sector if it is allocated or not
>     (bdrv_is_allocated returns a number of sectors that are known to be in the same
>     state as the first one, but it is not required to include all of them)
>     
>     This changes the output format of the alloc command so that a change to the
>     expected output of qemu-iotests 019 is necessary once this is included.
>     
>     Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> I guess this guy knows. :)

The commit message is talking about something different. Consider the
following image, x is allocated, . is unallocated:

xxx...xxx

bdrv_is_allocated(offset = 0, length = 9 * cluster_size) can tell you "2
clusters allocated", for example because after two clusters the L2 table
ended. I found this pretty confusing and instead expected that qemu-io
loops until it finds the first different cluster, i.e. the result would
always be "3 clusters allocated". This is what I think the quoted commit
(tried to) implement. (Yes, makes the existing code even more embarrassing)

Your code would output "6 clusters allocated", because it wouldn't care
about the unallocated clusters in the middle.

>> Do you think there's a use case for the sum of the whole area?
> 
> I think for tests it's more useful, you don't want to depend on the implementation
> of is_allocated.  That's what I can guess from the above commit message and from
> the way 019 uses alloc.

I think 019 would work with both.

Kevin

Re: [Qemu-devel] [PATCH 17/26] qemu-io: fix the alloc command

[Paolo Bonzini]

Il 08/05/2012 15:16, Kevin Wolf ha scritto:
> The commit message is talking about something different. Consider the
> following image, x is allocated, . is unallocated:
> 
> xxx...xxx
> 
> bdrv_is_allocated(offset = 0, length = 9 * cluster_size) can tell you "2
> clusters allocated", for example because after two clusters the L2 table
> ended. I found this pretty confusing and instead expected that qemu-io
> loops until it finds the first different cluster, i.e. the result would
> always be "3 clusters allocated". This is what I think the quoted commit
> (tried to) implement. (Yes, makes the existing code even more embarrassing)

No embarrassment, though I indeed didn't expect it to be broken by you. :)

Ok, I see now.  You would terminate the loop if is_allocated returns
something different from the result of the first call.

BTW I'm going to send the updated patches in a few minutes.  Last minute
testing found another bug (in HMP block_job_set_speed, which aborts).

>> I think for tests it's more useful, you don't want to depend on the implementation
>> of is_allocated.  That's what I can guess from the above commit message and from
>> the way 019 uses alloc.
>
> I think 019 would work with both.

Yes, because what you suggested also will not depend on the
implementation of is_allocated.

I'm inclined to keep code that is closest to what we have (even though
we have it by an odd series of events).  We can add the other one as a
switch later.

Paolo