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

[Kevin Wolf <kwolf@redhat.com>]

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

Am 26.05.2016 um 16:35 hat Eric Blake geschrieben:
> 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).

The obvious one would be translating your English into C:

    tail ? s->cluster_sectors - tail : 0

Another option which doesn't require an unsigned type would be
(s->cluster_sectors - tail) % s->cluster_sectors.

I'm okay with merging the "more interesting" version, though I must
admit that I had to read it twice.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]