Re: [Qemu-devel] [PATCH v2 for-2.10 09/16] block/qcow2: Generalize preallocate()

[Max Reitz]

On 03.04.2017 21:19, Philippe Mathieu-Daudé wrote:
> Hi Max,
> 
> On 04/03/2017 01:09 PM, Max Reitz wrote:
>> This patch adds two new parameters to the preallocate() function so we
>> will be able to use it not just for preallocating a new image but also
>> for preallocated image growth.
>>
>> The offset parameter allows the caller to specify a virtual offset from
>> which to start preallocating. For newly created images this is always 0,
>> but for preallocating growth this will be the old image length.
>>
>> The new_length parameter specifies the supposed new length of the image
>> (basically the "end offset" for preallocation). During image truncation,
>> bdrv_getlength() will return the old image length so we cannot rely on
>> its return value then.
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  block/qcow2.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 9fd220ec34..163d51ec65 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2034,17 +2034,23 @@ static int
>> qcow2_change_backing_file(BlockDriverState *bs,
>>      return qcow2_update_header(bs);
>>  }
>>
>> -static int preallocate(BlockDriverState *bs)
>> +/**
>> + * Preallocates metadata structures for data clusters between @offset
>> (in the
>> + * guest disk) and @new_length (which is thus generally the new guest
>> disk
>> + * size).
>> + *
>> + * Returns: 0 on success, -errno on failure.
>> + */
>> +static int preallocate(BlockDriverState *bs, int64_t offset, int64_t
>> new_length)
>>  {
>>      uint64_t bytes;
>> -    uint64_t offset;
>>      uint64_t host_offset = 0;
>>      unsigned int cur_bytes;
>>      int ret;
>>      QCowL2Meta *meta;
>>
>> -    bytes = bdrv_getlength(bs);
>> -    offset = 0;
> 
> why use `int64_t`? is it ok to have a negative offset?
> preallocate a negative length sound weird... even `bytes` is unsigned.

Right. That's actually a very good question. I can't remember why I
chose them to be signed.

Generally, it won't be an issue because the QEMU block layer will
generally not permit files to be longer than 2^63 (e.g.
bdrv_co_pwritev() and bdrv_co_preadv() take int64_t offsets).

But you're right, there is no reason for those parameters to be signed
here. I'll make them unsigned, thanks!

Max

>> +    assert(offset <= new_length);
>> +    bytes = new_length - offset;
>>
>>      while (bytes) {
>>          cur_bytes = MIN(bytes, INT_MAX);
>> @@ -2314,7 +2320,7 @@ static int qcow2_create2(const char *filename,
>> int64_t total_size,
>>      if (prealloc != PREALLOC_MODE_OFF) {
>>          BDRVQcow2State *s = blk_bs(blk)->opaque;
>>          qemu_co_mutex_lock(&s->lock);
>> -        ret = preallocate(blk_bs(blk));
>> +        ret = preallocate(blk_bs(blk), 0, total_size);
>>          qemu_co_mutex_unlock(&s->lock);
>>          if (ret < 0) {
>>              error_setg_errno(errp, -ret, "Could not preallocate
>> metadata");
>>

signature.asc
Description: OpenPGP digital signature