Re: [Qemu-devel] [PATCH 4/4] qemu-img: conditionally discard target on convert

[Peter Lieven]

Am 17.07.2013 um 12:27 schrieb Kevin Wolf <address@hidden>:

> Am 17.07.2013 um 12:21 hat Peter Lieven geschrieben:
>> 
>> Am 17.07.2013 um 11:58 schrieb Paolo Bonzini <address@hidden>:
>> 
>>> Il 17/07/2013 10:46, Kevin Wolf ha scritto:
>>>> Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:
>>>>> if a destination has has_zero_init = 0, but it supports
>>>>> discard zeroes use discard to convert the target
>>>>> into an all zero device.
>>>>> 
>>>>> Signed-off-by: Peter Lieven <address@hidden>
>>>> 
>>>> Wouldn't it be better to use bdrv_write_zeroes() and extend the
>>>> implementation of that to use discard internally in those block drivers
>>>> where it makes sense?
>>>> 
>>>> Because here you're not really discarding (i.e. don't care about the
>>>> sectors any more), but you want them to be zeroed.
>>> 
>>> I thought the same yesterday when reviewing the series, but I'm not
>>> convinced.
>>> 
>>> Discarding is not always the right way to write zeroes, because it can
>>> disrupt performance.  It may be fine when you are already going to write
>>> a sparse image (as is the case for qemu-img convert), but not in
>>> general.  So if you just used write_zeroes, it would have to fall under
>>> yet another -drive option (or an extension to "-drive discard").  I
>>> think what Peter did is a good compromise in the end.
>>> 
>>> BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always
>>> zeroes blocks, but is that true for unaligned operations?
>> 
>> Good question, I will pass it to ronnie. My guess is that the command will 
>> fail with
>> a check condition if it failed to unmap the data. From what Ronnie sent 
>> earlier
>> it should be guaranteed that the blocks are at least zero after the unmap 
>> command.
>> 
>> As for the qemu-img patch this shouldn't matter. It uses always blocks of 
>> bdi->max_unmap
>> which should be a multiple of the alignment. It also checks if sectors are 
>> deallocated
>> after the unmap afterwards. If the unmap fails it falls back to 
>> has_zero_init =1.
> 
> Well, you use bdrv_discard(), and ignoring discards is valid. Just
> another reason to use bdrv_write_zeroes() instead.

I use discard and afterwards check the provisioning state again. Just in case 
it silently failed for
whatever reason.

I am ok to have a general algorithm in bdrv_write_zeroes that is used as soon 
as the
block driver return discard_zeroes.

Would you be happy with that? I would tweak it that way that it uses discard to 
write zeroes
and double checks the provisioning status after the discard. If it hasn't 
turned to unmapped
real zeroes are written. I would further add code to honor max_unmap and 
discard_alignment
information.

Peter