Re: [Qemu-block] [PATCH v2 09/13] qed: Convert to bdrv_co_pwrite_zeroes()

[Eric Blake]

On 06/02/2016 05:16 AM, Kevin Wolf wrote:
> Am 01.06.2016 um 23:10 hat Eric Blake geschrieben:
>> Another step on our continuing quest to switch to byte-based
>> interfaces.
>>
>> Kill an abuse of the comma operator while at it (fortunately,
>> the semantics were still right).  Also, the test for requests
>> not aligned to clusters should be applied always, not just
>> when a backing file is present.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>  block/qed.c | 33 +++++++++++++++------------------
>>  1 file changed, 15 insertions(+), 18 deletions(-)

>> -        }
>> +    /* Fall back if the request is not */
> 
> ...aligned?

Yes, thanks.

> 
>> +    if (qed_offset_into_cluster(s, offset) ||
>> +        qed_offset_into_cluster(s, count)) {
>> +        return -ENOTSUP;
>>      }
> 
> This is obviously correct and almost as obviously suboptimal compared to
> the original version (we need cluster alignment with a backing file, but
> without a backing file, sector alignment would be enough).

Does QED support per-sector zeroing, or is it only per-cluster zero like
qcow2?

/me checks the spec

only per-cluster zeroing

> 
> But as this is QED, which is only supported for compatibility these
> days, simpler if slightly suboptimal code is okay with me.

Widening a request to cluster boundaries is only possible if you know
the cluster is otherwise unallocated or already reads as zeroes, and
unless qed tracks zero sectors (as opposed to zero clusters), I argue
that this was actually a bug fix, even when the request was
sector-aligned, since we lack the check for cluster remains
unallocated/zero before widening, the way qcow2 does it.  Sub-optimal
but safe is better than incorrectly optimized.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature