From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58538) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqPOf-0006rc-LY for qemu-devel@nongnu.org; Mon, 17 Nov 2014 11:43:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqPOa-0005SP-Og for qemu-devel@nongnu.org; Mon, 17 Nov 2014 11:43:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37804) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqPOa-0005SC-Au for qemu-devel@nongnu.org; Mon, 17 Nov 2014 11:43:20 -0500 Message-ID: <546A25A2.3030801@redhat.com> Date: Mon, 17 Nov 2014 09:43:14 -0700 From: Eric Blake MIME-Version: 1.0 References: <1416219514-22530-1-git-send-email-armbru@redhat.com> <1416219514-22530-4-git-send-email-armbru@redhat.com> In-Reply-To: <1416219514-22530-4-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="5UcTWMfHVdFrIenptMeEE3HJwD2Oek8E8" Subject: Re: [Qemu-devel] [PATCH v3 for-2.2 3/3] raw-posix: The SEEK_HOLE code is flawed, rewrite it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, tony@bakeyournoodle.com, mreitz@redhat.com, stefanha@redhat.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --5UcTWMfHVdFrIenptMeEE3HJwD2Oek8E8 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/17/2014 03:18 AM, Markus Armbruster wrote: > On systems where SEEK_HOLE in a trailing hole seeks to EOF (Solaris, > but not Linux), try_seek_hole() reports trailing data instead. Maybe worth a comment that this is not fatal, but also not optimal. >=20 > Additionally, unlikely lseek() failures are treated badly: >=20 > * When SEEK_HOLE fails, try_seek_hole() reports trailing data. For > -ENXIO, there's in fact a trailing hole. Can happen only when > something truncated the file since we opened it. >=20 > * When SEEK_HOLE succeeds, SEEK_DATA fails, and SEEK_END succeeds, > then try_seek_hole() reports a trailing hole. This is okay only > when SEEK_DATA failed with -ENXIO (which means the non-trailing hole > found by SEEK_HOLE has since become trailing somehow). For other > failures (unlikely), it's wrong. >=20 > * When SEEK_HOLE succeeds, SEEK_DATA fails, SEEK_END fails (unlikely), > then try_seek_hole() reports bogus data [-1,start), which its caller > raw_co_get_block_status() turns into zero sectors of data. Could > theoretically lead to infinite loops in code that attempts to scan > data vs. hole forward. >=20 > Rewrite from scratch, with very careful comments. Thanks for the careful commit message as well as the careful comments :) >=20 > Signed-off-by: Markus Armbruster > --- > block/raw-posix.c | 111 +++++++++++++++++++++++++++++++++++++++++-----= -------- > 1 file changed, 85 insertions(+), 26 deletions(-) >=20 > @@ -1542,25 +1600,26 @@ static int64_t coroutine_fn raw_co_get_block_st= atus(BlockDriverState *bs, > nb_sectors =3D DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SI= ZE); > } > =20 > + } else 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); I think we are safe for now that no file system supports holes smaller than 512 bytes, so (hole - start) should always be a non-zero multiple of sectors. Similarly for the hole case of (data - start). Maybe it's worth assert(*pnum > 0) to ensure that we never hit a situation where we go into an infinite loop where we aren't progressing because pnum is never advancing to the next sector? But that would be okay as a separate patch, and I don't want to delay getting _this_ patch into 2.2. Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --5UcTWMfHVdFrIenptMeEE3HJwD2Oek8E8 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 iQEcBAEBCAAGBQJUaiWiAAoJEKeha0olJ0NqPS0H+wbMXXyihRI8hlOlO4H65FQI xDjeHtB35t7n5MvnxcqBRCj1HR4ffq1dGvZVhtYQoEbqTIbHQl14vXNoniNdqIP3 +taWVN8O7kXM7ufl6TR6+BQoFiWoayaI9W9lJ7LoxAR3bHvSGH77fjNV4F42QVva gRunNU37Xn6adxnIA6lA53C4PGqzN2ZiN7tAYJU1fDO5m/dK2kmELoSye7LO9sRQ XxBF8HeDXBdHxou7VmpxieE7ZmT6ihksL02zLkUGYBLC8nY4SAOAaPkxoK+ubaFf oaIfl4PU4ktehGCBFHrepCwLEGa/7kei0q9BYZHwZpCtWUA1POfa1uuCwuuqN4I= =+edP -----END PGP SIGNATURE----- --5UcTWMfHVdFrIenptMeEE3HJwD2Oek8E8--