Re: [Qemu-devel] [PATCH v3 04/16] qemu-img: Expose PreallocMode for resizing

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

On 2017-05-30 22:57, Eric Blake wrote:
> On 05/26/2017 11:55 AM, Max Reitz wrote:
>> Add a --preallocation command line option to qemu-img resize which can
>> be used to set the PreallocMode parameter of blk_truncate().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qemu-img.c    | 33 ++++++++++++++++++++++++++++++---
>>  qemu-img.texi |  7 ++++++-
>>  2 files changed, 36 insertions(+), 4 deletions(-)
> 
>> @@ -3553,8 +3566,16 @@ static int img_resize(int argc, char **argv)
>>          goto out;
>>      }
>>  
>> +    current_size = blk_getlength(blk);
>> +    if (current_size < 0) {
>> +        error_report("Failed to inquire current image length: %s",
>> +                     strerror(-current_size));
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>>      if (relative) {
>> -        total_size = blk_getlength(blk) + n * relative;
>> +        total_size = current_size + n * relative;
> 
> You snuck in a bug fix here (reporting failure, rather than using a
> bogus total_size, if querying the size fails).  Please mention that in
> the commit message.
> 
>> @@ -541,6 +541,11 @@ After using this command to grow a disk image, you must use file system and
>>  partitioning tools inside the VM to actually begin using the new space on the
>>  device.
>>  
>> +When growing an image, the @code{--preallocation} option may be used to specify
>> +how the additional image area should be allocated on the host.  See the format
>> +description in the @code{NOTES} section which values are allowed.  Using this
>> +option may result in more data being allocated than necessary.
> 
> Should we tone it down a bit by saying 'slightly more data'?  (We'd
> rather over-estimate than fall short, but our over-estimation will
> probably be < 1% off, and not something drastic like an order of
> magnitude off).

I wanted to be on the safe side for future updates for other formats
(where I'm not sure how well we can do this).

I somehow have the feeling that XFS preallocation may play its game,
too, although I can't seem to trigger it...

Anyway, as long as we can justify, say, ~10 % as "slightly", I'm OK with
adding it.

> With the improved commit message,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Will do, and as always: Thanks!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]