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

[Kevin Wolf]

Am 17.07.2013 um 11:58 hat Paolo Bonzini geschrieben:
> 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").

Maybe we need a flag to bdrv_write_zeroes() that specifies whether to
use discard or not, kind of like the unmap bit in WRITE SAME.

On the other hand, I think you're right that this is really policy,
and as such shouldn't be hardcoded based on our guesses, but be
configurable.

> I think what Peter did is a good compromise in the end.

At the very least it must become a separate function. img_convert() is
already too big and too deeply nested.

> BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always
> zeroes blocks, but is that true for unaligned operations?

SBC-3 Rev. 35e, 5.16 READ CAPACITY (16) command:

    "A logical block provisioning read zeros (LBPRZ) bit set to one
    indicates that, for read commands specifying an unmapped LBA (see
    4.7.4.5), the device server returns user data set to zero [...]"

So it depends on the block provisioning state of the LBA, not on the
operations that were performed on it.

5.28 UNMAP command:

    If the ANCHOR bit in the CDB is set to zero, and the logical unit is
    thin provisioned (see 4.7.3.3), then the logical block provisioning
    state for each specified LBA:

    a) should become deallocated;
    b) may become anchored; or
    c) may remain unchanged.

So with UNMAP, I think you don't have any guarantees that the LBA
becomes unmapped and therefore zeroed. It could just keep its current
data. No matter whether your request was aligned or not.

Kevin