[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 5/5] qcow2: Catch more unaligned write_zero i
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v3 5/5] qcow2: Catch more unaligned write_zero into zero cluster |
Date: |
Thu, 2 Jun 2016 12:14:50 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
pgpdlDa6CKozd.pgp
Description: PGP signature
- Re: [Qemu-devel] [PATCH v3 5/5] qcow2: Catch more unaligned write_zero into zero cluster,
Kevin Wolf <=