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
pgpXArKtOe6cd.pgp
Description: PGP signature
