Re: [Qemu-devel] [PATCH 4/4] qemu-img: conditionally discard target on convert

[Peter Lieven <pl@kamp.de>]

On 18.07.2013 12:56, Paolo Bonzini wrote:
> Il 18/07/2013 12:44, Peter Lieven ha scritto:
>> On 18.07.2013 12:24, Paolo Bonzini wrote:
>>> Il 18/07/2013 11:23, Kevin Wolf ha scritto:
>>>> Am 17.07.2013 um 19:48 hat Peter Lieven geschrieben:
>>>>> Am 17.07.2013 um 19:04 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>>>>>
>>>>>> Il 17/07/2013 19:02, Peter Lieven ha scritto:
>>>>>>> For Disks we always use read/write16 so i think we Should also use
>>>>>>> writesame16. Or not?
>>>>>> Yes.
>>>>>>
>>>>>> Remember you can still use UNMAP if LBPRZ=0.
>>>>> I can always use it if writesame is not available, but in this case
>>>>> bdi->discard_zeroes must be 0.
>>>>>
>>>>> Maybe we should call it discard_writes_zeroes or similar.
>>>>>
>>>>> Discard_zeroes is sth that should only indicate if lbprz == 1. At
>>>>> least if we refer to the Linux ioctl. We could include both in BDI.
>>>> Maybe what we really should do is to define different operations (with
>>>> an exact behaviour) instead of having one bdrv_discard() and then adding
>>>> flags everywhere to tell what the operation is doing exactly.
>>> A BDRV_MAY_UNMAP flag for bdrv_write_zeroes?
>> I thought that we wanted to add a paramter to the BDI (call it
>> write_zeroes_w_discard).
> I also thought so, but I like Kevin's idea of not shoehorning it in
> bdrv_discard().  Extending bdrv_write_zeroes is a better API.
>
> So far we've avoided "discard zeroes" semantics in QEMU (device models
> are also careful not to expose that).  Since the only sane way to
> implement what you want is to use the SCSI WRITE SAME command, adding
> flags to bdrv_write_zeroes will even be easier because the mapping with
> SCSI is natural: discard = UNMAP, write_zeroes = WRITE SAME (either
> without or with the UNMAP bit).
>
>> If this is set the bdrv MUST accept a flag to bdrv_discard() lets call
>> it BDRV_DISCARD_WRITE_ZEROES
>> and he has to ensure that all sectors specified in bdrv_discard() read
>> as zero after the operation.
>>
>> If this flag is not set (e.g. when the OS issues a normal discard) the
>> operation may still silently fail with
>> undefined provisioning state and content of the specified sectors.
> But if you set BDRV_DISCARD_WRITE_ZEROES, then you always need a
> fallback to bdrv_write_zeroes.  Why not just call bdrv_write_zeroes to
> begin with?  That's why extending bdrv_write_zeroes is preferable.
In this case wo do not need a flag to the function at all. If the
driver sets bdi->write_zeroes_w_discard = 1 then bdrv_write_zeroes
can use bdrv_discard to write zeroes and the driver has to
ensure that all is zero afterwards.

If the driver would have a better method of writing zeroes than
discard it simply should not set bdi->write_zeroes_w_discard = 1.

Peter