Re: [Qemu-devel] [PATCHv3 00/20] block: logical block provisioning enhancements

[Peter Lieven <pl@kamp.de>]

On 08.10.2013 10:59, Stefan Hajnoczi wrote:
> On Tue, Oct 8, 2013 at 10:01 AM, Peter Lieven <pl@kamp.de> wrote:
>> On 08.10.2013 09:02, Stefan Hajnoczi wrote:
>>> On Mon, Oct 7, 2013 at 11:42 AM, Paolo Bonzini <pbonzini@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 ?

Peter