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

[Stefan Hajnoczi]

On Tue, Oct 8, 2013 at 11:12 AM, Peter Lieven <address@hidden> wrote:
> On 08.10.2013 10:59, Stefan Hajnoczi wrote:
>>
>> On Tue, Oct 8, 2013 at 10:01 AM, Peter Lieven <address@hidden> wrote:
>>>
>>> On 08.10.2013 09:02, Stefan Hajnoczi wrote:
>>>>
>>>> On Mon, Oct 7, 2013 at 11:42 AM, Paolo Bonzini <address@hidden>
>>>> 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