On Tue, Oct 8, 2013 at 10:01 AM, Peter Lieven <p...@kamp.de> wrote: > On 08.10.2013 09:02, Stefan Hajnoczi wrote: >> >> On Mon, Oct 7, 2013 at 11:42 AM, Paolo Bonzini <pbonz...@redhat.com> >> wrote: >>> >>> Il 07/10/2013 10:42, Stefan Hajnoczi ha scritto: >>>> >>>> Could you make bdrv_co_write_zeroes() always use UNMAP, if possible, and >>>> avoid adding the new BDRV_REQ_MAY_UNMAP flag? While reading the first >>>> few patches in this series I wondered why there is a need to expose >>>> flags at all... >>>> >>>> Sometimes it is useful to distinguish between zeroing at the image >>>> format level from discarding at the device level, but I don't think we >>>> make use of that yet. I'd prefer to keep the interface simple for now >>>> and add flags later, if necessary. >>>> >>>> Or maybe I just missed something ;) >>> >>> The flag is needed to implement the right semantics for the SCSI WRITE >>> SAME command, which are: >>> >>> - if the UNMAP bit is off, always write the sectors (that's >>> bdrv_aio_write_zeroes without BDRV_REQ_MAY_UNMAP if the payload is zero, >>> otherwise it's emulated with bdrv_aio_writev) >>> >>> - if the target can "discard and write the specified payload", you can >>> discard, else you must write the sectors with the correct payload >>> (that's bdrv_aio_write_zeroes with BDRV_REQ_MAY_UNMAP). >>> >>> Contrast this with the UNMAP command, which does not make any guarantee >>> on the content of the sectors after the command is completed (a few >>> months ago we agreed that, even if you have discard_zeroes=true in the >>> target, it is fine for UNMAP to do nothing). >> >> Okay, then let's keep the patches to expose the flag. > > Okay, then I can keep those. > > Can you give a short hint if my approach with brdv_make_empty is what > you want? I would like to not change the parameters, so use > BDRV_REQ_MAY_UNMAP > unconditionally. > > int bdrv_make_empty(BlockDriverState *bs)
The semantics of bdrv_make_empty() today are: deallocate all data in the top layer of the image file. If there is a backing file, reads will fall back to the backing file. The semantics that you want are zeroing the entire disk image (efficiently, when possible). A flags argument is needed to support both of sets of semantics. If you don't like that, then I suggest creating a new function called bdrv_make_zero(). Stefan