diff mbox

[01/17] cow: make reads go at a decent speed

Message ID 1372862071-28225-2-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini July 3, 2013, 2:34 p.m. UTC 
Do not do two reads for each sector; load each sector of the bitmap
and use bitmap operations to process it.

Writes are still dog slow!

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c | 54 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

Comments

Fam Zheng July 4, 2013, 2:20 a.m. UTC | #1 
On Wed, 07/03 16:34, Paolo Bonzini wrote:
> Do not do two reads for each sector; load each sector of the bitmap
> and use bitmap operations to process it.
> 
> Writes are still dog slow!
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/cow.c | 54 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index 1cc2e89..204451e 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -126,18 +126,31 @@ static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
>      return 0;
>  }
>  
> -static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
> +#define BITS_PER_BITMAP_SECTOR (512 * 8)
> +
> +/* Cannot use bitmap.c on big-endian machines.  */
> +static int cow_test_bit(int64_t bitnum, const uint8_t *bitmap)
>  {
> -    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
> -    uint8_t bitmap;
> -    int ret;
> +    return (bitmap[bitnum / 8] & (1 << (bitnum & 7))) != 0;
> +}
>  
> -    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
> -    if (ret < 0) {
> -       return ret;
> +static int cow_find_streak(const uint8_t *bitmap, int value, int start, int nb_sectors)
I think type bool is better for 'value' as you don't booleanize it.  And
also int64_t for start?
> +{
> +    int streak_value = value ? 0xFF : 0;
> +    int last = MIN(start + nb_sectors, BITS_PER_BITMAP_SECTOR);
> +    int bitnum = start;
> +    while (bitnum < last) {
> +        if ((bitnum & 7) == 0 && bitmap[bitnum / 8] == streak_value) {
> +            bitnum += 8;
> +            continue;
> +        }
> +        if (cow_test_bit(bitnum, bitmap) == value) {
> +            bitnum++;
> +            continue;
> +        }
> +        break;
>      }
> -
> -    return !!(bitmap & (1 << (bitnum % 8)));
> +    return MIN(bitnum, last) - start;
>  }
>  
>  /* Return true if first block has been changed (ie. current version is
> @@ -146,23 +159,20 @@ static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
>  static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
>          int64_t sector_num, int nb_sectors, int *num_same)
>  {
> +    int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
> +    uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
> +    uint8_t bitmap[512];
> +    int ret;
>      int changed;
>  
> -    if (nb_sectors == 0) {
> -	*num_same = nb_sectors;
> -	return 0;
> -    }
> -
> -    changed = is_bit_set(bs, sector_num);
> -    if (changed < 0) {
> -        return 0; /* XXX: how to return I/O errors? */
> -    }
> -
> -    for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) {
> -	if (is_bit_set(bs, sector_num + *num_same) != changed)
> -	    break;
> +    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
> +    if (ret < 0) {
> +        return ret;
>      }
>  
> +    bitnum &= BITS_PER_BITMAP_SECTOR - 1;
> +    changed = cow_test_bit(bitnum, bitmap);
> +    *num_same = cow_find_streak(bitmap, changed, bitnum, nb_sectors);
>      return changed;
>  }
>  
> -- 
> 1.8.2.1
> 
> 
>
Paolo Bonzini July 4, 2013, 8:08 a.m. UTC | #2 
Il 04/07/2013 04:20, Fam Zheng ha scritto:
> On Wed, 07/03 16:34, Paolo Bonzini wrote:
>> Do not do two reads for each sector; load each sector of the bitmap
>> and use bitmap operations to process it.
>>
>> Writes are still dog slow!
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block/cow.c | 54 ++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 32 insertions(+), 22 deletions(-)
>>
>> diff --git a/block/cow.c b/block/cow.c
>> index 1cc2e89..204451e 100644
>> --- a/block/cow.c
>> +++ b/block/cow.c
>> @@ -126,18 +126,31 @@ static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
>>      return 0;
>>  }
>>  
>> -static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
>> +#define BITS_PER_BITMAP_SECTOR (512 * 8)
>> +
>> +/* Cannot use bitmap.c on big-endian machines.  */
>> +static int cow_test_bit(int64_t bitnum, const uint8_t *bitmap)
>>  {
>> -    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
>> -    uint8_t bitmap;
>> -    int ret;
>> +    return (bitmap[bitnum / 8] & (1 << (bitnum & 7))) != 0;
>> +}
>>  
>> -    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
>> -    if (ret < 0) {
>> -       return ret;
>> +static int cow_find_streak(const uint8_t *bitmap, int value, int start, int nb_sectors)
> I think type bool is better for 'value' as you don't booleanize it.  And
> also int64_t for start?

start is always between 0 and BITS_PER_BITMAP_SECTOR.

"value" here is a bit value, so 0 or 1 rather than true or false.  I
prefer to keep it as "int", but it can be changed.

Paolo

>> +{
>> +    int streak_value = value ? 0xFF : 0;
>> +    int last = MIN(start + nb_sectors, BITS_PER_BITMAP_SECTOR);
>> +    int bitnum = start;
>> +    while (bitnum < last) {
>> +        if ((bitnum & 7) == 0 && bitmap[bitnum / 8] == streak_value) {
>> +            bitnum += 8;
>> +            continue;
>> +        }
>> +        if (cow_test_bit(bitnum, bitmap) == value) {
>> +            bitnum++;
>> +            continue;
>> +        }
>> +        break;
>>      }
>> -
>> -    return !!(bitmap & (1 << (bitnum % 8)));
>> +    return MIN(bitnum, last) - start;
>>  }
>>  
>>  /* Return true if first block has been changed (ie. current version is
>> @@ -146,23 +159,20 @@ static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
>>  static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
>>          int64_t sector_num, int nb_sectors, int *num_same)
>>  {
>> +    int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
>> +    uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
>> +    uint8_t bitmap[512];
>> +    int ret;
>>      int changed;
>>  
>> -    if (nb_sectors == 0) {
>> -	*num_same = nb_sectors;
>> -	return 0;
>> -    }
>> -
>> -    changed = is_bit_set(bs, sector_num);
>> -    if (changed < 0) {
>> -        return 0; /* XXX: how to return I/O errors? */
>> -    }
>> -
>> -    for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) {
>> -	if (is_bit_set(bs, sector_num + *num_same) != changed)
>> -	    break;
>> +    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
>> +    if (ret < 0) {
>> +        return ret;
>>      }
>>  
>> +    bitnum &= BITS_PER_BITMAP_SECTOR - 1;
>> +    changed = cow_test_bit(bitnum, bitmap);
>> +    *num_same = cow_find_streak(bitmap, changed, bitnum, nb_sectors);
>>      return changed;
>>  }
>>  
>> -- 
>> 1.8.2.1
>>
>>
>>
>
Stefan Hajnoczi July 5, 2013, 9:09 a.m. UTC | #3 
On Wed, Jul 03, 2013 at 04:34:15PM +0200, Paolo Bonzini wrote:
> @@ -146,23 +159,20 @@ static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
>  static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
>          int64_t sector_num, int nb_sectors, int *num_same)
>  {
> +    int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
> +    uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
> +    uint8_t bitmap[512];

Please use BDRV_SECTOR_SIZE, you used it one line above but then
switched to literal 512.
diff mbox

Patch

diff --git a/block/cow.c b/block/cow.c
index 1cc2e89..204451e 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -126,18 +126,31 @@  static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
     return 0;
 }
 
-static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
+#define BITS_PER_BITMAP_SECTOR (512 * 8)
+
+/* Cannot use bitmap.c on big-endian machines.  */
+static int cow_test_bit(int64_t bitnum, const uint8_t *bitmap)
 {
-    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
-    uint8_t bitmap;
-    int ret;
+    return (bitmap[bitnum / 8] & (1 << (bitnum & 7))) != 0;
+}
 
-    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
-    if (ret < 0) {
-       return ret;
+static int cow_find_streak(const uint8_t *bitmap, int value, int start, int nb_sectors)
+{
+    int streak_value = value ? 0xFF : 0;
+    int last = MIN(start + nb_sectors, BITS_PER_BITMAP_SECTOR);
+    int bitnum = start;
+    while (bitnum < last) {
+        if ((bitnum & 7) == 0 && bitmap[bitnum / 8] == streak_value) {
+            bitnum += 8;
+            continue;
+        }
+        if (cow_test_bit(bitnum, bitmap) == value) {
+            bitnum++;
+            continue;
+        }
+        break;
     }
-
-    return !!(bitmap & (1 << (bitnum % 8)));
+    return MIN(bitnum, last) - start;
 }
 
 /* Return true if first block has been changed (ie. current version is
@@ -146,23 +159,20 @@  static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
 static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *num_same)
 {
+    int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
+    uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
+    uint8_t bitmap[512];
+    int ret;
     int changed;
 
-    if (nb_sectors == 0) {
-	*num_same = nb_sectors;
-	return 0;
-    }
-
-    changed = is_bit_set(bs, sector_num);
-    if (changed < 0) {
-        return 0; /* XXX: how to return I/O errors? */
-    }
-
-    for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) {
-	if (is_bit_set(bs, sector_num + *num_same) != changed)
-	    break;
+    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
+    if (ret < 0) {
+        return ret;
     }
 
+    bitnum &= BITS_PER_BITMAP_SECTOR - 1;
+    changed = cow_test_bit(bitnum, bitmap);
+    *num_same = cow_find_streak(bitmap, changed, bitnum, nb_sectors);
     return changed;
 }