Patchwork [08/12] qemu-img: conditionally zero out target on convert

login
register
mail settings
Submitter Peter Lieven
Date Sept. 13, 2013, 10:25 a.m.
Message ID <1379067909-22984-9-git-send-email-pl@kamp.de>
Download mbox | patch
Permalink /patch/274715/
State New
Headers show

Comments

Peter Lieven - Sept. 13, 2013, 10:25 a.m.
if the target has_zero_init = 0, but supports efficiently
writing zeroes by unmapping we call bdrv_zeroize to
avoid fully allocating the target. this currently
is designed especially for iscsi.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)
Paolo Bonzini - Sept. 13, 2013, 10:36 a.m.
Il 13/09/2013 12:25, Peter Lieven ha scritto:
> if the target has_zero_init = 0, but supports efficiently
> writing zeroes by unmapping we call bdrv_zeroize to
> avoid fully allocating the target. this currently
> is designed especially for iscsi.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c |   22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 3e5e388..6eaddc6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1354,7 +1354,8 @@ static int img_convert(int argc, char **argv)
>          }
>      }
>  
> -    flags = BDRV_O_RDWR;
> +    flags = BDRV_O_RDWR | BDRV_O_UNMAP;

I think this should be a new command-line flag.

Paolo

> +
>      ret = bdrv_parse_cache_flags(cache, &flags);
>      if (ret < 0) {
>          error_report("Invalid cache option: %s", cache);
> @@ -1386,12 +1387,13 @@ static int img_convert(int argc, char **argv)
>          }
>      }
>  
> +    ret = bdrv_get_info(out_bs, &bdi);
> +    if (ret < 0) {
> +        error_report("could not get block driver info");
> +        goto out;
> +    }
> +
>      if (compress) {
> -        ret = bdrv_get_info(out_bs, &bdi);
> -        if (ret < 0) {
> -            error_report("could not get block driver info");
> -            goto out;
> -        }
>          cluster_size = bdi.cluster_size;
>          if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) {
>              error_report("invalid cluster size");
> @@ -1470,6 +1472,14 @@ static int img_convert(int argc, char **argv)
>      } else {
>          int has_zero_init = bdrv_has_zero_init(out_bs);
>  
> +        if (!has_zero_init && !out_baseimg && bdi.discard_write_zeroes) {
> +            ret = bdrv_zeroize(out_bs, BDRV_REQ_MAY_UNMAP);
> +            if (ret < 0) {
> +                goto out;
> +            }
> +            has_zero_init = 1;
> +        }
> +
>          sector_num = 0; // total number of sectors converted so far
>          nb_sectors = total_sectors - sector_num;
>          if (nb_sectors != 0) {
>
Peter Lieven - Sept. 13, 2013, 10:46 a.m.
On 13.09.2013 12:36, Paolo Bonzini wrote:
> Il 13/09/2013 12:25, Peter Lieven ha scritto:
>> if the target has_zero_init = 0, but supports efficiently
>> writing zeroes by unmapping we call bdrv_zeroize to
>> avoid fully allocating the target. this currently
>> is designed especially for iscsi.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   qemu-img.c |   22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 3e5e388..6eaddc6 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1354,7 +1354,8 @@ static int img_convert(int argc, char **argv)
>>           }
>>       }
>>   
>> -    flags = BDRV_O_RDWR;
>> +    flags = BDRV_O_RDWR | BDRV_O_UNMAP;
> I think this should be a new command-line flag.
In an earlier version there where no objections. I think it would
make the usage of qemu-img convert more complicated. For
most targets has_zero_init is 1 anyway.

Peter
Eric Blake - Sept. 13, 2013, 6:25 p.m.
On 09/13/2013 04:36 AM, Paolo Bonzini wrote:
> Il 13/09/2013 12:25, Peter Lieven ha scritto:
>> if the target has_zero_init = 0, but supports efficiently
>> writing zeroes by unmapping we call bdrv_zeroize to
>> avoid fully allocating the target. this currently
>> is designed especially for iscsi.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  qemu-img.c |   22 ++++++++++++++++------
>>  1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 3e5e388..6eaddc6 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1354,7 +1354,8 @@ static int img_convert(int argc, char **argv)
>>          }
>>      }
>>  
>> -    flags = BDRV_O_RDWR;
>> +    flags = BDRV_O_RDWR | BDRV_O_UNMAP;
> 
> I think this should be a new command-line flag.

I agree - while 'sparse by default' may be reasonable, it is also
feasible to want a mode that guarantees expansion rather than unmapped
or sparse.
Peter Lieven - Sept. 13, 2013, 7:48 p.m.
Am 13.09.2013 20:25, schrieb Eric Blake:
> On 09/13/2013 04:36 AM, Paolo Bonzini wrote:
>> Il 13/09/2013 12:25, Peter Lieven ha scritto:
>>> if the target has_zero_init = 0, but supports efficiently
>>> writing zeroes by unmapping we call bdrv_zeroize to
>>> avoid fully allocating the target. this currently
>>> is designed especially for iscsi.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>  qemu-img.c |   22 ++++++++++++++++------
>>>  1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index 3e5e388..6eaddc6 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -1354,7 +1354,8 @@ static int img_convert(int argc, char **argv)
>>>          }
>>>      }
>>>  
>>> -    flags = BDRV_O_RDWR;
>>> +    flags = BDRV_O_RDWR | BDRV_O_UNMAP;
>> I think this should be a new command-line flag.
> I agree - while 'sparse by default' may be reasonable, it is also
> feasible to want a mode that guarantees expansion rather than unmapped
> or sparse.
>
Ok, so do you find the proposed -S 0 bei Paolo a good choice?
If this is supplied I would go as far as completly setting
has_zero_init = 0 also for targets which default to 1. This
would guaranteed exspansion and full allocation for all drivers.

Peter
Eric Blake - Sept. 13, 2013, 7:52 p.m.
On 09/13/2013 01:48 PM, Peter Lieven wrote:
>>>> +    flags = BDRV_O_RDWR | BDRV_O_UNMAP;
>>> I think this should be a new command-line flag.
>> I agree - while 'sparse by default' may be reasonable, it is also
>> feasible to want a mode that guarantees expansion rather than unmapped
>> or sparse.
>>
> Ok, so do you find the proposed -S 0 bei Paolo a good choice?
> If this is supplied I would go as far as completly setting
> has_zero_init = 0 also for targets which default to 1. This
> would guaranteed exspansion and full allocation for all drivers.

Yeah, works for me - an optional mode that guarantees full allocation.
Paolo Bonzini - Sept. 16, 2013, 11:01 a.m.
Il 13/09/2013 21:48, Peter Lieven ha scritto:
> Am 13.09.2013 20:25, schrieb Eric Blake:
>> On 09/13/2013 04:36 AM, Paolo Bonzini wrote:
>>> Il 13/09/2013 12:25, Peter Lieven ha scritto:
>>>> if the target has_zero_init = 0, but supports efficiently
>>>> writing zeroes by unmapping we call bdrv_zeroize to
>>>> avoid fully allocating the target. this currently
>>>> is designed especially for iscsi.
>>>>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>  qemu-img.c |   22 ++++++++++++++++------
>>>>  1 file changed, 16 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>> index 3e5e388..6eaddc6 100644
>>>> --- a/qemu-img.c
>>>> +++ b/qemu-img.c
>>>> @@ -1354,7 +1354,8 @@ static int img_convert(int argc, char **argv)
>>>>          }
>>>>      }
>>>>  
>>>> -    flags = BDRV_O_RDWR;
>>>> +    flags = BDRV_O_RDWR | BDRV_O_UNMAP;
>>> I think this should be a new command-line flag.
>> I agree - while 'sparse by default' may be reasonable, it is also
>> feasible to want a mode that guarantees expansion rather than unmapped
>> or sparse.
>>
> Ok, so do you find the proposed -S 0 bei Paolo a good choice?
> If this is supplied I would go as far as completly setting
> has_zero_init = 0 also for targets which default to 1. This
> would guaranteed exspansion and full allocation for all drivers.

Sounds good.

Paolo

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 3e5e388..6eaddc6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1354,7 +1354,8 @@  static int img_convert(int argc, char **argv)
         }
     }
 
-    flags = BDRV_O_RDWR;
+    flags = BDRV_O_RDWR | BDRV_O_UNMAP;
+
     ret = bdrv_parse_cache_flags(cache, &flags);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -1386,12 +1387,13 @@  static int img_convert(int argc, char **argv)
         }
     }
 
+    ret = bdrv_get_info(out_bs, &bdi);
+    if (ret < 0) {
+        error_report("could not get block driver info");
+        goto out;
+    }
+
     if (compress) {
-        ret = bdrv_get_info(out_bs, &bdi);
-        if (ret < 0) {
-            error_report("could not get block driver info");
-            goto out;
-        }
         cluster_size = bdi.cluster_size;
         if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) {
             error_report("invalid cluster size");
@@ -1470,6 +1472,14 @@  static int img_convert(int argc, char **argv)
     } else {
         int has_zero_init = bdrv_has_zero_init(out_bs);
 
+        if (!has_zero_init && !out_baseimg && bdi.discard_write_zeroes) {
+            ret = bdrv_zeroize(out_bs, BDRV_REQ_MAY_UNMAP);
+            if (ret < 0) {
+                goto out;
+            }
+            has_zero_init = 1;
+        }
+
         sector_num = 0; // total number of sectors converted so far
         nb_sectors = total_sectors - sector_num;
         if (nb_sectors != 0) {