Re: [Qemu-devel] [PATCH 2/3] COW: Extend checking allocated bits to beyond one sector

[Paolo Bonzini <pbonzini@redhat.com>]

Il 06/11/2013 13:23, Charlie Shepherd ha scritto:
> cow_co_is_allocated() only checks one sector's worth of allocated bits before returning. This is
> allowed but (slightly) inefficient, so extend it to check all of the file's metadata sectors.
> 
> Signed-off-by: Charlie Shepherd <charlie@ctshepherd.com>
> ---
>  block/cow.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index 66f1478..4a081cb 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -152,18 +152,34 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
>  {
>      int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
>      uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
> -    uint8_t bitmap[BDRV_SECTOR_SIZE];
> -    int ret;
> -    int changed;
> +    bool first = true;
> +    int changed, same = 0;
>  
> -    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
> -    if (ret < 0) {
> -        return ret;
> -    }
> +    do {
> +        int ret;
> +        uint8_t bitmap[BDRV_SECTOR_SIZE];
> +
> +        bitnum &= BITS_PER_BITMAP_SECTOR - 1;
> +        int sector_bits = MIN(nb_sectors, BITS_PER_BITMAP_SECTOR - bitnum);
> +
> +        ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        if (first) {
> +            changed = cow_test_bit(bitnum, bitmap);
> +            first = false;
> +        }
> +
> +        same += cow_find_streak(bitmap, changed, bitnum, nb_sectors);
> +
> +        bitnum += sector_bits;
> +        nb_sectors -= sector_bits;
> +        offset += BDRV_SECTOR_SIZE;
> +    } while (nb_sectors);
>  
> -    bitnum &= BITS_PER_BITMAP_SECTOR - 1;
> -    changed = cow_test_bit(bitnum, bitmap);
> -    *num_same = cow_find_streak(bitmap, changed, bitnum, nb_sectors);
> +    *num_same = same;
>      return changed;
>  }
>  
> 

This one is good.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>