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

[Kevin Wolf]

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.

Kevin