[Qemu-devel] [PATCH v3 0/2] COW: Speed up writes

[Qemu-devel] [PATCH v3 0/2] COW: Speed up writes

[Charlie Shepherd]

v3:
  - Refix cow_update_bitmap and squash patches 1 & 3 together to ensuring that we only flush if
    necessary, patch 1 on its own would change this causing a regression.
v2:
  - Fix bit position calculations in cow_update_bitmap
  - Add necessary check in cow_set_bits

Following on from Paolo's commits 26ae980 and 276cbc7, this patchset
implements some changes he recommended earlier which I didn't previously have
time to do while on GSoC.

Charlie Shepherd (2):
  COW: Speed up writes
  COW: Extend checking allocated bits to beyond one sector

 block/cow.c | 123 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 75 insertions(+), 48 deletions(-)

-- 
1.8.4.rc3

[Qemu-devel] [PATCH v3 1/2] COW: Speed up writes

[Charlie Shepherd]

Process a whole sector's worth of COW bits by reading a sector, setting the bits after skipping
any already set bits, then writing it out again. Make sure we only flush once before writing
metadata, and only if we need to write metadata.

Signed-off-by: Charlie Shepherd <charlie@ctshepherd.com>
---
 block/cow.c | 87 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 49 insertions(+), 38 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index 909c3e7..bf447fd 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -103,40 +103,18 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
     return ret;
 }
 
-/*
- * XXX(hch): right now these functions are extremely inefficient.
- * We should just read the whole bitmap we'll need in one go instead.
- */
-static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum, bool *first)
+static inline void cow_set_bits(uint8_t *bitmap, int start, int64_t nb_sectors)
 {
-    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
-    uint8_t bitmap;
-    int ret;
-
-    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
-    if (ret < 0) {
-       return ret;
-    }
-
-    if (bitmap & (1 << (bitnum % 8))) {
-        return 0;
-    }
-
-    if (*first) {
-        ret = bdrv_flush(bs->file);
-        if (ret < 0) {
-            return ret;
+    int64_t bitnum = start, last = start + nb_sectors;
+    while (bitnum < last) {
+        if ((bitnum & 7) == 0 && bitnum + 8 <= last) {
+            bitmap[bitnum / 8] = 0xFF;
+            bitnum += 8;
+            continue;
         }
-        *first = false;
+        bitmap[bitnum/8] |= (1 << (bitnum % 8));
+        bitnum++;
     }
-
-    bitmap |= (1 << (bitnum % 8));
-
-    ret = bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap));
-    if (ret < 0) {
-       return ret;
-    }
-    return 0;
 }
 
 #define BITS_PER_BITMAP_SECTOR (512 * 8)
@@ -204,18 +182,51 @@ static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
 static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
         int nb_sectors)
 {
-    int error = 0;
-    int i;
+    int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
+    uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
     bool first = true;
 
-    for (i = 0; i < nb_sectors; i++) {
-        error = cow_set_bit(bs, sector_num + i, &first);
-        if (error) {
-            break;
+    for ( ; nb_sectors;
+            bitnum += sector_bits,
+            nb_sectors -= sector_bits,
+            offset += BDRV_SECTOR_SIZE) {
+        int ret, set;
+        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;
+        }
+
+        /* Skip over any already set bits */
+        set = cow_find_streak(bitmap, 1, bitnum, sector_bits);
+        bitnum += set;
+        sector_bits -= set;
+        nb_sectors -= set;
+        if (!sector_bits) {
+            continue;
+        }
+
+        if (first) {
+            ret = bdrv_flush(bs->file);
+            if (ret < 0) {
+                return ret;
+            }
+            first = false;
+        }
+
+        cow_set_bits(bitmap, bitnum, sector_bits);
+
+        ret = bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap));
+        if (ret < 0) {
+            return ret;
         }
     }
 
-    return error;
+    return 0;
 }
 
 static int coroutine_fn cow_read(BlockDriverState *bs, int64_t sector_num,
-- 
1.8.4.rc3

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

[Charlie Shepherd]

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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index bf447fd..f9e2293 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;
 }
 
-- 
1.8.4.rc3

Re: [Qemu-devel] [PATCH v3 1/2] COW: Speed up writes

[Paolo Bonzini]

Il 06/11/2013 16:59, Charlie Shepherd ha scritto:
> Process a whole sector's worth of COW bits by reading a sector, setting the bits after skipping
> any already set bits, then writing it out again. Make sure we only flush once before writing
> metadata, and only if we need to write metadata.
> 
> Signed-off-by: Charlie Shepherd <charlie@ctshepherd.com>
> ---
>  block/cow.c | 87 ++++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 49 insertions(+), 38 deletions(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index 909c3e7..bf447fd 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -103,40 +103,18 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
>      return ret;
>  }
>  
> -/*
> - * XXX(hch): right now these functions are extremely inefficient.
> - * We should just read the whole bitmap we'll need in one go instead.
> - */
> -static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum, bool *first)
> +static inline void cow_set_bits(uint8_t *bitmap, int start, int64_t nb_sectors)
>  {
> -    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
> -    uint8_t bitmap;
> -    int ret;
> -
> -    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
> -    if (ret < 0) {
> -       return ret;
> -    }
> -
> -    if (bitmap & (1 << (bitnum % 8))) {
> -        return 0;
> -    }
> -
> -    if (*first) {
> -        ret = bdrv_flush(bs->file);
> -        if (ret < 0) {
> -            return ret;
> +    int64_t bitnum = start, last = start + nb_sectors;
> +    while (bitnum < last) {
> +        if ((bitnum & 7) == 0 && bitnum + 8 <= last) {
> +            bitmap[bitnum / 8] = 0xFF;
> +            bitnum += 8;
> +            continue;
>          }
> -        *first = false;
> +        bitmap[bitnum/8] |= (1 << (bitnum % 8));
> +        bitnum++;
>      }
> -
> -    bitmap |= (1 << (bitnum % 8));
> -
> -    ret = bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap));
> -    if (ret < 0) {
> -       return ret;
> -    }
> -    return 0;
>  }
>  
>  #define BITS_PER_BITMAP_SECTOR (512 * 8)
> @@ -204,18 +182,51 @@ static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
>  static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
>          int nb_sectors)
>  {
> -    int error = 0;
> -    int i;
> +    int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
> +    uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
>      bool first = true;
>  
> -    for (i = 0; i < nb_sectors; i++) {
> -        error = cow_set_bit(bs, sector_num + i, &first);
> -        if (error) {
> -            break;
> +    for ( ; nb_sectors;
> +            bitnum += sector_bits,
> +            nb_sectors -= sector_bits,
> +            offset += BDRV_SECTOR_SIZE) {
> +        int ret, set;
> +        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;
> +        }
> +
> +        /* Skip over any already set bits */
> +        set = cow_find_streak(bitmap, 1, bitnum, sector_bits);
> +        bitnum += set;
> +        sector_bits -= set;
> +        nb_sectors -= set;
> +        if (!sector_bits) {
> +            continue;
> +        }
> +
> +        if (first) {
> +            ret = bdrv_flush(bs->file);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +            first = false;
> +        }
> +
> +        cow_set_bits(bitmap, bitnum, sector_bits);
> +
> +        ret = bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap));
> +        if (ret < 0) {
> +            return ret;
>          }
>      }
>  
> -    return error;
> +    return 0;
>  }
>  
>  static int coroutine_fn cow_read(BlockDriverState *bs, int64_t sector_num,
> 

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

Re: [Qemu-devel] [PATCH v3 1/2] COW: Speed up writes

[Charlie Shepherd]

On 06/11/2013 16:17, Paolo Bonzini wrote:
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks for your help with this series!

Charlie

Re: [Qemu-devel] [PATCH v3 0/2] COW: Speed up writes

[Kevin Wolf]

Am 06.11.2013 um 16:59 hat Charlie Shepherd geschrieben:
> v3:
>   - Refix cow_update_bitmap and squash patches 1 & 3 together to ensuring that we only flush if
>     necessary, patch 1 on its own would change this causing a regression.
> v2:
>   - Fix bit position calculations in cow_update_bitmap
>   - Add necessary check in cow_set_bits
> 
> Following on from Paolo's commits 26ae980 and 276cbc7, this patchset
> implements some changes he recommended earlier which I didn't previously have
> time to do while on GSoC.
> 
> Charlie Shepherd (2):
>   COW: Speed up writes
>   COW: Extend checking allocated bits to beyond one sector
> 
>  block/cow.c | 123 ++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 75 insertions(+), 48 deletions(-)

Thanks, applied to the block-next branch for 1.8.

Your lines in the commit message were a bit too long, please try to have
a line break after ~72 characters in future patches (I fixed it up
manually for this one).

Kevin

Re: [Qemu-devel] [PATCH v3 1/2] COW: Speed up writes

[Kevin Wolf]

Am 06.11.2013 um 16:59 hat Charlie Shepherd geschrieben:
> Process a whole sector's worth of COW bits by reading a sector, setting the bits after skipping
> any already set bits, then writing it out again. Make sure we only flush once before writing
> metadata, and only if we need to write metadata.
> 
> Signed-off-by: Charlie Shepherd <charlie@ctshepherd.com>
> ---
>  block/cow.c | 87 ++++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 49 insertions(+), 38 deletions(-)

> @@ -204,18 +182,51 @@ static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
>  static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
>          int nb_sectors)
>  {
> -    int error = 0;
> -    int i;
> +    int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
> +    uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
>      bool first = true;
>  
> -    for (i = 0; i < nb_sectors; i++) {
> -        error = cow_set_bit(bs, sector_num + i, &first);
> -        if (error) {
> -            break;
> +    for ( ; nb_sectors;
> +            bitnum += sector_bits,
> +            nb_sectors -= sector_bits,
> +            offset += BDRV_SECTOR_SIZE) {

block/cow.c: In function 'cow_update_bitmap':
block/cow.c:206:23: error: 'sector_bits' undeclared (first use in this function)

Kevin

Re: [Qemu-devel] [PATCH v3 1/2] COW: Speed up writes

[Charlie Shepherd]

On 13/11/2013 12:59, Kevin Wolf wrote:
> Am 06.11.2013 um 16:59 hat Charlie Shepherd geschrieben:
>> Process a whole sector's worth of COW bits by reading a sector, setting the bits after skipping
>> any already set bits, then writing it out again. Make sure we only flush once before writing
>> metadata, and only if we need to write metadata.
>>
>> Signed-off-by: Charlie Shepherd <charlie@ctshepherd.com>
>> ---
>>   block/cow.c | 87 ++++++++++++++++++++++++++++++++++---------------------------
>>   1 file changed, 49 insertions(+), 38 deletions(-)
>> @@ -204,18 +182,51 @@ static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
>>   static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
>>           int nb_sectors)
>>   {
>> -    int error = 0;
>> -    int i;
>> +    int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
>> +    uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
>>       bool first = true;
>>   
>> -    for (i = 0; i < nb_sectors; i++) {
>> -        error = cow_set_bit(bs, sector_num + i, &first);
>> -        if (error) {
>> -            break;
>> +    for ( ; nb_sectors;
>> +            bitnum += sector_bits,
>> +            nb_sectors -= sector_bits,
>> +            offset += BDRV_SECTOR_SIZE) {
> block/cow.c: In function 'cow_update_bitmap':
> block/cow.c:206:23: error: 'sector_bits' undeclared (first use in this function)

Sorry I should have caught that, resending a new version.


Charlie