Re: [Qemu-devel] [PATCH v3 5/5] qcow2: Catch more unaligned write_zero into zero cluster

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2305 bytes --]

On 05/26/2016 07:41 AM, Denis V. Lunev wrote:
> On 05/26/2016 06:48 AM, Eric Blake wrote:
>> is_zero_cluster() and is_zero_cluster_top_locked() are used only
>> by qcow2_co_write_zeroes().  The former is too broad (we don't
>> care if the sectors we are about to overwrite are non-zero, only
>> that all other sectors in the cluster are zero), so it needs to
>> be called up to twice but with smaller limits - rename it along
>> with adding the neeeded parameter.  The latter can be inlined for
>> more compact code.
>>
>> The testsuite change shows that we now have a sparser top file
>> when an unaligned write_zeroes overwrites the only portion of
>> the backing file with data.
>>
>> Based on a patch proposal by Denis V. Lunev.
>>

>> -
>> -        if (!is_zero_cluster(bs, sector_num)) {
>> +        /* check whether remainder of cluster already reads as zero */
>> +        if (!(is_zero_sectors(bs, cl_start, head) &&
>> +              is_zero_sectors(bs, sector_num + nb_sectors,
>> +                              -tail & (s->cluster_sectors - 1)))) {
> 
> can we have cluster_sectors != 2^n?

No. cluster_sectors is an inherent property of the qcow2 file format:


         20 - 23:   cluster_bits
                    Number of bits that are used for addressing an offset
                    within a cluster (1 << cluster_bits is the cluster
size).
                    Must not be less than 9 (i.e. 512 byte clusters).


As the file format uses a bit shift value, you are guaranteed to have a
power of two amount of sectors within a cluster.

If you prefer, I could have written '-tail % s->cluster_sectors', but as
% on a negative signed integer gives different results than what you get
for an unsigned number, I felt that & was nicer than % for making it
more obvious that I'm grabbing particular bits.

If you can think of any cleaner expression that represents the number of
sectors occurring after the tail until the next cluster boundary, I'm
game; the hardest part is that when tail is 0, we want the number passed
to is_zero_sectors() to also be 0, not s->cluster_sectors (so the naive
's->cluster_sectors - tail' is wrong).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]