From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45825) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xokq7-0003IB-Lk for qemu-devel@nongnu.org; Wed, 12 Nov 2014 22:13:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xokq2-0005lm-Dv for qemu-devel@nongnu.org; Wed, 12 Nov 2014 22:12:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36163) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xokq2-0005lb-5a for qemu-devel@nongnu.org; Wed, 12 Nov 2014 22:12:50 -0500 Message-ID: <5463EC70.1030107@redhat.com> Date: Wed, 12 Nov 2014 17:25:36 -0600 From: Eric Blake MIME-Version: 1.0 References: <1415820422-17796-1-git-send-email-armbru@redhat.com> <1415820422-17796-3-git-send-email-armbru@redhat.com> In-Reply-To: <1415820422-17796-3-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="m963JdrXR3KaUXER69B1DvdHXHG0PiNn6" Subject: Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, tony@bakeyournoodle.com, stefanha@redhat.com, mreitz@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --m963JdrXR3KaUXER69B1DvdHXHG0PiNn6 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/12/2014 01:27 PM, Markus Armbruster wrote: > Commit 5500316 (May 2012) implemented raw_co_is_allocated() as > follows: >=20 > Thus, the FIEMAP code executes rarely. Makes it a nice hidey-hole for > bugs. Worse, bugs hiding there can theoretically bite even on a host > that has SEEK_HOLE/SEEK_DATA. >=20 > I don't want to worry about this crap, not even theoretically. Get > rid of it, then clean up the mess, including spotty error checking. Sounds reasonable to me. It's rather a big patch (both nuking a bad interface and rewriting the use of the good interface) that might have been better as two commits, but I can live with it. > Signed-off-by: Markus Armbruster > --- > block/raw-posix.c | 128 ++++++++++++++++++++--------------------------= -------- > 1 file changed, 47 insertions(+), 81 deletions(-) >=20 > +/* > + * Find allocation range in @bs around offset @start. > + * If @start is in a hole, store @start in @hole and the end of the > + * hole in @data. > + * If @start is in a data, store @start to @data, and the end of the > + * data to @hole. > + * If we can't find out, pretend there are no holes. > + */ > +static void find_allocation(BlockDriverState *bs, off_t start, > + off_t *data, off_t *hole) Sounds like a good contract interface. > + /* in hole, end not yet known */ > + offs =3D lseek(s->fd, start, SEEK_DATA); > + if (offs < 0) { > + /* no idea where the hole ends, give up (unlikely to happen) *= / > + goto dunno; > + } > + assert(offs >=3D start); > + *hole =3D start; > + *data =3D offs; This assertion feels like an off-by-one. The same offset cannot be both a hole and data (except in some racy situation where some other process is writing data to that offset in between our two lseek calls, but that's already in no-man's land because no one else should be writing the file while qemu has it open). Is it worth using 'assert(offs > start)' instead? > + ret =3D BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; > + find_allocation(bs, start, &data, &hole); > + if (data =3D=3D start) { > /* On a data extent, compute sectors to the end of the extent.= */ > *pnum =3D MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE); > - return ret | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start= ; > } else { > /* On a hole, compute sectors to the beginning of the next ext= ent. */ > + assert(hole =3D=3D start); > *pnum =3D MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE); > - return ret | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start= ; > + ret |=3D BDRV_BLOCK_ZERO; > } > + return ret; The old code omits BDRV_BLOCK_DATA on a hole. Why are you adding it here, and why are you not mentioning it in the commit message? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --m963JdrXR3KaUXER69B1DvdHXHG0PiNn6 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUY+xxAAoJEKeha0olJ0NqyzMIAIRtlAwKZW6E/5bUZ8B/LU57 oiUSgYtqmvTgiQZHr82B3NPS8021QKlqcEcJPX9q3/xKCEBj+bn7NRkztxXR0sBE EezIS34n82s3Pod5uiWgm4ciJ8XC/QSf57LxnjnQhe/Rf6nZWkFHTYQ1aIgBZek4 bmEwSHTtH9pQAw7yyyXiPaWIukja7nO4C7k/09JUEFH+AFSssF5OrbHF37LhVPca OXRM3Li0n/cFdJpe47S1sYclORvK4fNTIBv0bw7s20Fud1GKeVjTPvBLf/NHQ+U9 AwZ3wn3TyS/y3GZ8Z58oG5L/0XP48pZmhJya1DP5AV0+hBXzvR2eKtheiy1HiSM= =g+YI -----END PGP SIGNATURE----- --m963JdrXR3KaUXER69B1DvdHXHG0PiNn6--