Re: [Qemu-devel] [PATCH 02/17] cow: make writes go at a less indecent speed

[Paolo Bonzini]

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 <address@hidden>
>> ---
>>  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
>>
>>
>>
>