On Tue, Oct 8, 2013 at 11:12 AM, Peter Lieven <p...@kamp.de> wrote: > On 08.10.2013 10:59, Stefan Hajnoczi wrote: >> >> 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(). > > Ok, that is what I would like to do. In this case I only have to rename > bdrv_zeroize to bdrv_make_zero. Ok ?
Yes, please. Stefan
