Patchwork [6/8] qemu-img: use bdrv_write_zeroes to write zeroes

login
register
mail settings
Submitter Peter Lieven
Date June 22, 2013, 8:58 p.m.
Message ID <1371934712-11714-7-git-send-email-pl@kamp.de>
Download mbox | patch
Permalink /patch/253430/
State New
Headers show

Comments

Peter Lieven - June 22, 2013, 8:58 p.m.
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
Paolo Bonzini - June 24, 2013, 2:33 p.m.
Il 22/06/2013 22:58, Peter Lieven ha scritto:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 809b4f1..5aa53ab 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
>                     If the output is to a host device, we also write out
>                     sectors that are entirely 0, since whatever data was
>                     already there is garbage, not 0s. */
> -                if (!has_zero_init || out_baseimg ||
> -                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {
> -                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
> +                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
> +                if (!has_zero_init || out_baseimg || allocated) {
> +                    if (allocated || out_baseimg) {
> +                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
> +                    } else {
> +                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);

I think it should still do the write only if !has_zero_init.

Paolo

> +                    }
>                      if (ret < 0) {
>                          error_report("error while writing sector %" PRId64
>                                       ": %s", sector_num, strerror(-ret));
>
Peter Lieven - June 24, 2013, 4:17 p.m.
Am 24.06.2013 16:33, schrieb Paolo Bonzini:
> Il 22/06/2013 22:58, Peter Lieven ha scritto:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  qemu-img.c |   10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 809b4f1..5aa53ab 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
>>                     If the output is to a host device, we also write out
>>                     sectors that are entirely 0, since whatever data was
>>                     already there is garbage, not 0s. */
>> -                if (!has_zero_init || out_baseimg ||
>> -                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {
>> -                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
>> +                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
>> +                if (!has_zero_init || out_baseimg || allocated) {
>> +                    if (allocated || out_baseimg) {
>> +                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
>> +                    } else {
>> +                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);
> I think it should still do the write only if !has_zero_init.
With this I am still in trouble with iSCSI, but Kevin pointed out another
possible solution for the allocating everything problem.

a) let iscsi_create discard the whole device if lbpz && lbprz
b) return 1 for has_zero_init if lbprz.

What do you think?

Peter
>
> Paolo
>
>> +                    }
>>                      if (ret < 0) {
>>                          error_report("error while writing sector %" PRId64
>>                                       ": %s", sector_num, strerror(-ret));
>>
Paolo Bonzini - June 24, 2013, 4:25 p.m.
Il 24/06/2013 18:17, Peter Lieven ha scritto:
> Am 24.06.2013 16:33, schrieb Paolo Bonzini:
>> Il 22/06/2013 22:58, Peter Lieven ha scritto:
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>  qemu-img.c |   10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index 809b4f1..5aa53ab 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
>>>                     If the output is to a host device, we also write out
>>>                     sectors that are entirely 0, since whatever data was
>>>                     already there is garbage, not 0s. */
>>> -                if (!has_zero_init || out_baseimg ||
>>> -                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {
>>> -                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
>>> +                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
>>> +                if (!has_zero_init || out_baseimg || allocated) {
>>> +                    if (allocated || out_baseimg) {
>>> +                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
>>> +                    } else {
>>> +                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);
>> I think it should still do the write only if !has_zero_init.
> With this I am still in trouble with iSCSI, but Kevin pointed out another
> possible solution for the allocating everything problem.
> 
> a) let iscsi_create discard the whole device if lbpz && lbprz
> b) return 1 for has_zero_init if lbprz.
> 
> What do you think?

Fine by me (but the discard may take a looong time! :)).

Paolo
Peter Lieven - June 24, 2013, 4:33 p.m.
Am 24.06.2013 18:25, schrieb Paolo Bonzini:
> Il 24/06/2013 18:17, Peter Lieven ha scritto:
>> Am 24.06.2013 16:33, schrieb Paolo Bonzini:
>>> Il 22/06/2013 22:58, Peter Lieven ha scritto:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>  qemu-img.c |   10 +++++++---
>>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>> index 809b4f1..5aa53ab 100644
>>>> --- a/qemu-img.c
>>>> +++ b/qemu-img.c
>>>> @@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
>>>>                     If the output is to a host device, we also write out
>>>>                     sectors that are entirely 0, since whatever data was
>>>>                     already there is garbage, not 0s. */
>>>> -                if (!has_zero_init || out_baseimg ||
>>>> -                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {
>>>> -                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
>>>> +                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
>>>> +                if (!has_zero_init || out_baseimg || allocated) {
>>>> +                    if (allocated || out_baseimg) {
>>>> +                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
>>>> +                    } else {
>>>> +                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);
>>> I think it should still do the write only if !has_zero_init.
>> With this I am still in trouble with iSCSI, but Kevin pointed out another
>> possible solution for the allocating everything problem.
>>
>> a) let iscsi_create discard the whole device if lbpz && lbprz
>> b) return 1 for has_zero_init if lbprz.
>>
>> What do you think?
> Fine by me (but the discard may take a looong time! :)).
This was also my concern, but if a target advertises logical block
provisioning it should be able to discard blocks fast (and not overwrite all of
them or sth like that).

Peter
>
> Paolo
>
Peter Lieven - June 24, 2013, 6:46 p.m.
Am 24.06.2013 18:25, schrieb Paolo Bonzini:
> Il 24/06/2013 18:17, Peter Lieven ha scritto:
>> Am 24.06.2013 16:33, schrieb Paolo Bonzini:
>>> Il 22/06/2013 22:58, Peter Lieven ha scritto:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>  qemu-img.c |   10 +++++++---
>>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>> index 809b4f1..5aa53ab 100644
>>>> --- a/qemu-img.c
>>>> +++ b/qemu-img.c
>>>> @@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
>>>>                     If the output is to a host device, we also write out
>>>>                     sectors that are entirely 0, since whatever data was
>>>>                     already there is garbage, not 0s. */
>>>> -                if (!has_zero_init || out_baseimg ||
>>>> -                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {
>>>> -                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
>>>> +                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
>>>> +                if (!has_zero_init || out_baseimg || allocated) {
>>>> +                    if (allocated || out_baseimg) {
>>>> +                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
>>>> +                    } else {
>>>> +                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);
>>> I think it should still do the write only if !has_zero_init.
>> With this I am still in trouble with iSCSI, but Kevin pointed out another
>> possible solution for the allocating everything problem.
>>
>> a) let iscsi_create discard the whole device if lbpz && lbprz
>> b) return 1 for has_zero_init if lbprz.
>>
>> What do you think?
> Fine by me (but the discard may take a looong time! :)).
fyi: i have created an optimized unmap function which transmits the
maximum allowed unmap_list length (read from block_limits page).

for 1TB it takes approx. 1 minute on my storage.

i will include this in v2 of the series and also add the optimization
to the other unmap calls (which currently do not check max_unmap
size at all).

Peter

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 809b4f1..5aa53ab 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1513,9 +1513,13 @@  static int img_convert(int argc, char **argv)
                    If the output is to a host device, we also write out
                    sectors that are entirely 0, since whatever data was
                    already there is garbage, not 0s. */
-                if (!has_zero_init || out_baseimg ||
-                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {
-                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
+                if (!has_zero_init || out_baseimg || allocated) {
+                    if (allocated || out_baseimg) {
+                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                    } else {
+                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);
+                    }
                     if (ret < 0) {
                         error_report("error while writing sector %" PRId64
                                      ": %s", sector_num, strerror(-ret));