On 2018-02-01 14:34, Anton Nefedov wrote:
> On 31/1/2018 8:31 PM, Max Reitz wrote:
>> On 2018-01-30 13:34, Anton Nefedov wrote:
>>> Offtop: does REQ_ZERO_WRITE override REQ_WRITE_COMPRESSED in this
>>> function? at least with !REQ_MAY_UNMAP it looks wrong
>>
>> Looks like zero detection will indeed override compression.  I think
>> that was intended, but I don't even have an opinion either way.
>>
>> Of course, it wouldn't be so nice if you tried to compress something and
>> then, because the zero write failed, you actually write uncompressed
>> zeroes...  But since zero detection is an optional feature, it might be
>> your own fault if you enable it when you want compression anyway, and if
>> you write to some format/protocol combination that doesn't allow zero
>> writes.
>>
>> Max
>>
> 
> Is it detection only? I guess in case the caller sets
> (REQ_ZERO_WRITE | REQ_COMPRESSED) it also fires here.

True, but that's the caller's "fault".  One of those things has to take
precedence.

And I've just noticed that when bdrv_co_do_pwrite_zeroes() falls back to
a bounce buffer, it passes the original flags (without
BDRV_REQ_ZERO_WRITE) to bdrv_driver_pwritev(), so compression will take
effect then.  So the only result is that we first try a zero write, and
if that fails, we do a compressed write.  That sounds reasonable to me.

(I mean, we could do it the other way around, but I firstly I don't
think it matters much and secondly it's probably better this way because
I'd suspect zero writes to be more efficient than compressing the bounce
buffer; at least that's how it is for qcow2.)

Max

> Looks like noone does that though, but for example backup might;
> it supports compression, and it also does a zero detection itself and
> calls write_zeroes() when it can.
> It sets REQ_MAY_UNMAP which should indeed override a compression, but
> unmap might be not supported.
> 
> /Anton