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

[Stefan Hajnoczi]

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().

Stefan