[02/17] cow: make writes go at a less indecent speed

Only sync once per write, rather than once per sector.

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

On Wed, 07/03 16:34, Paolo Bonzini wrote:
> Only sync once per write, rather than once per sector.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/cow.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index 204451e..133e596 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -106,7 +106,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags)
>   * 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)
> +static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum, bool *first)
Why flush _before first_ write, rather than (more intuitively) flush
_after last_ write? And personally I think "bool sync" makes a better
signature than "bool *first", although it's not that critical as
cow_update_bitmap is the only caller.
>  {
>      uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
>      uint8_t bitmap;
> @@ -117,9 +117,21 @@ static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
>         return ret;
>      }
>  
> +    if (bitmap & (1 << (bitnum % 8))) {
> +        return 0;
> +    }
> +
> +    if (*first) {
> +        ret = bdrv_flush(bs->file);
> +        if (ret < 0) {
> +           return ret;
> +        }
> +        *first = false;
> +    }
> +
>      bitmap |= (1 << (bitnum % 8));
>  
> -    ret = bdrv_pwrite_sync(bs->file, offset, &bitmap, sizeof(bitmap));
> +    ret = bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap));
>      if (ret < 0) {
>         return ret;
>      }
> @@ -181,9 +193,10 @@ static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
>  {
>      int error = 0;
>      int i;
> +    bool first = true;
>  
>      for (i = 0; i < nb_sectors; i++) {
> -        error = cow_set_bit(bs, sector_num + i);
> +        error = cow_set_bit(bs, sector_num + i, &first);
>          if (error) {
>              break;
>          }
> -- 
> 1.8.2.1
> 
> 
>

Il 04/07/2013 04:40, Fam Zheng ha scritto:
> On Wed, 07/03 16:34, Paolo Bonzini wrote:
>> Only sync once per write, rather than once per sector.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block/cow.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/cow.c b/block/cow.c
>> index 204451e..133e596 100644
>> --- a/block/cow.c
>> +++ b/block/cow.c
>> @@ -106,7 +106,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags)
>>   * 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)
>> +static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum, bool *first)
> Why flush _before first_ write, rather than (more intuitively) flush
> _after last_ write?

Because you have to flush the data before you start writing the
metadata.  Flushing the metadata can be done when the guest issues a flush.

This ensures that, in case of a power loss, the metadata will never
refer to data that hasn't been written.

Paolo

 And personally I think "bool sync" makes a better
> signature than "bool *first", although it's not that critical as
> cow_update_bitmap is the only caller.
>>  {
>>      uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
>>      uint8_t bitmap;
>> @@ -117,9 +117,21 @@ static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
>>         return ret;
>>      }
>>  
>> +    if (bitmap & (1 << (bitnum % 8))) {
>> +        return 0;
>> +    }
>> +
>> +    if (*first) {
>> +        ret = bdrv_flush(bs->file);
>> +        if (ret < 0) {
>> +           return ret;
>> +        }
>> +        *first = false;
>> +    }
>> +
>>      bitmap |= (1 << (bitnum % 8));
>>  
>> -    ret = bdrv_pwrite_sync(bs->file, offset, &bitmap, sizeof(bitmap));
>> +    ret = bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap));
>>      if (ret < 0) {
>>         return ret;
>>      }
>> @@ -181,9 +193,10 @@ static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
>>  {
>>      int error = 0;
>>      int i;
>> +    bool first = true;
>>  
>>      for (i = 0; i < nb_sectors; i++) {
>> -        error = cow_set_bit(bs, sector_num + i);
>> +        error = cow_set_bit(bs, sector_num + i, &first);
>>          if (error) {
>>              break;
>>          }
>> -- 
>> 1.8.2.1
>>
>>
>>
>