From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40205) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8PeZ-00067C-10 for qemu-devel@nongnu.org; Thu, 02 Jun 2016 06:15:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8PeW-0004Yz-S8 for qemu-devel@nongnu.org; Thu, 02 Jun 2016 06:15:01 -0400 Date: Thu, 2 Jun 2016 12:14:50 +0200 From: Kevin Wolf Message-ID: <20160602101450.GA6867@noname.redhat.com> References: <1464234529-13018-1-git-send-email-eblake@redhat.com> <1464234529-13018-6-git-send-email-eblake@redhat.com> <5746FCFC.2060708@openvz.org> <574709AD.4020805@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Nq2Wo0NMKNjxTN9z" Content-Disposition: inline In-Reply-To: <574709AD.4020805@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 5/5] qcow2: Catch more unaligned write_zero into zero cluster List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: "Denis V. Lunev" , qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz --Nq2Wo0NMKNjxTN9z Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >> >=20 > >> - > >> - 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)))) { > >=20 > > can we have cluster_sectors !=3D 2^n? >=20 > No. cluster_sectors is an inherent property of the qcow2 file format: >=20 >=20 > 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). >=20 >=20 > As the file format uses a bit shift value, you are guaranteed to have a > power of two amount of sectors within a cluster. >=20 > 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. >=20 > 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 --Nq2Wo0NMKNjxTN9z Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJXUAcaAAoJEH8JsnLIjy/W430P/jNRz5qM6dzlCI5uwTdsk8nM YQKiztA4xiwQNbFEhASBhMCbqD8saI7KuZw0TQJSFVvynRiU7rsXvpWPlj9zIdzN 9bzU7eJjnpNPsURI+ylk2pLZfzFxn8UEw1+tMoLbw+EzJkV4//QeESbNPuW8GwGf UsI78THUXo+MeR7p8irf6B4y9paeATYibQ3IJfqZkPqAG/BbsJKKevZ9oPs1sDK6 U1tgLGE9Yi57OIH9HIV5r66OdLU1WraAxrFk6t6TFCZHKdCgFTOxyvirY/I5VgFs bT+95GXHWl6RcRAzXNa+XAPJNEAIecOja5TvahOOP50aMWjSK6RUNALbKz/wLxX4 yO/WVw5eIj3jJ4eswqoNUjAhHdkHtISnTSbzJjHfLdYTDkUiFye2XUDjco3pQPKr pful3iJSkU15bNR8lp1izsQZrtkT4NVSCibuNmbrf7jMb3lV4gDGJRRcBPY8q2re +Stf1vTVUx/E9UYsiAwl/0LQpVP79TVfKtnwP9YaywpcnSAYp9J20o0xW1ud1t9U J9bZgje5O1EZtENbbeeqtGI0IzdreA0klKyNzDT7HORASLx41prJNNWGspQrw+rA UaxQOpyKiaW5vp0jcO31kq3QeTW5yZ8XeKVvtgiKnDmZzX6uFv2NFIRHOmpxyBJF EHIf8iTVySRsMYVlsNwJ =H6Wx -----END PGP SIGNATURE----- --Nq2Wo0NMKNjxTN9z--