Re: [Qemu-block] [PATCH v2 4/7] qcow2: Split do_perform_cow() into _read(), _encrypt() and _write()

[Alberto Garcia]

On Fri 09 Jun 2017 04:53:05 PM CEST, Eric Blake wrote:
> Let's suppose we have a guest issuing 512-byte aligned requests and a
> host that requires 4k alignment; and the guest does an operation that
> needs a COW with one sector at both the front and end of the cluster.
>
>> @@ -760,22 +776,59 @@ static int perform_cow(BlockDriverState *bs, 
>> QCowL2Meta *m)
>>      BDRVQcow2State *s = bs->opaque;
>>      Qcow2COWRegion *start = &m->cow_start;
>>      Qcow2COWRegion *end = &m->cow_end;
>> +    unsigned buffer_size = start->nb_bytes + end->nb_bytes;
>
> This sets buffer_size to 1024 initially.
>
>> +    uint8_t *start_buffer, *end_buffer;
>>      int ret;
>>  
>> +    assert(start->nb_bytes <= UINT_MAX - end->nb_bytes);
>> +
>>      if (start->nb_bytes == 0 && end->nb_bytes == 0) {
>>          return 0;
>>      }
>>  
>> +    /* Reserve a buffer large enough to store the data from both the
>> +     * start and end COW regions */
>> +    start_buffer = qemu_try_blockalign(bs, buffer_size);
>
> This is going to allocate a bigger buffer, namely one that is at least
> 4k in size (at least, that's my understanding - the block device is
> able to track its preferred IO size/alignment).
>
>> +    if (start_buffer == NULL) {
>> +        return -ENOMEM;
>> +    }
>> +    /* The part of the buffer where the end region is located */
>> +    end_buffer = start_buffer + start->nb_bytes;
>
> But now end_buffer does not have optimal alignment.  In the old code,
> we called qemu_try_blockalign() twice, so that both read()s were
> called on a 4k boundary; but now, the end read() is called unaligned
> to a 4k boundary.  Of course, since we're only reading 512 bytes,
> instead of 4k, it MIGHT not matter, but I don't know if we are going
> to cause a bounce buffer to come into play that we could otherwise
> avoid if we were smarter with our alignments.  Is that something we
> need to analyze further, or even possibly intentionally over-allocate
> our buffer to ensure optimal read alignments?

I think you're right, I actually thought about this but forgot it when
preparing the series for publishing.

If I'm not wrong it should be simply a matter of replacing

   buffer_size = start->nb_bytes + end->nb_bytes;

with

   buffer_size = QEMU_ALIGN_UP(start->nb_bytes, bdrv_opt_mem_align(bs))
                 + end->nb_bytes;

and then

    end_buffer = start_buffer + buffer_size - end->nb_bytes;

    (this one we do anyway in patch 5)

Berto