From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43943) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XpRWm-0007zt-4W for qemu-devel@nongnu.org; Fri, 14 Nov 2014 19:47:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XpRWh-0001pe-2D for qemu-devel@nongnu.org; Fri, 14 Nov 2014 19:47:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35658) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XpRWg-0001pV-Ni for qemu-devel@nongnu.org; Fri, 14 Nov 2014 19:47:42 -0500 Message-ID: <5466A2A5.8070703@redhat.com> Date: Fri, 14 Nov 2014 17:47:33 -0700 From: Eric Blake MIME-Version: 1.0 References: <1415873823-13844-1-git-send-email-armbru@redhat.com> <1415873823-13844-4-git-send-email-armbru@redhat.com> <20141113130327.GD3933@noname.redhat.com> <5464C59A.4000602@redhat.com> <5464CE42.4000809@redhat.com> <5464D291.4000303@redhat.com> <871tp62aff.fsf@blackfin.pond.sub.org> In-Reply-To: <871tp62aff.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Iet2bIw2S5iacrKVcMVtHUoR3CCewtQRv" Subject: Re: [Qemu-devel] [PATCH v2 3/4] raw-posix: Fix try_seek_hole()'s handling of SEEK_DATA failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , famz@redhat.com, tony@bakeyournoodle.com, qemu-devel@nongnu.org, mreitz@redhat.com, stefanha@redhat.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Iet2bIw2S5iacrKVcMVtHUoR3CCewtQRv Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/14/2014 06:12 AM, Markus Armbruster wrote: >> 0-length file: >> lseek(fd, 0, SEEK_HOLE) =3D> -1 ENXIO >> lseek(fd, 0, SEEK_DATA) =3D> -1 ENXIO >> conclusion: 0 is at EOF >=20 > Isn't this a special case of the next one? >=20 >> file of any size: >> lseek(fd, size_or_larger, SEEK_HOLE) =3D> -1 ENXIO >> lseek(fd, size_or_larger, SEEK_DATA) =3D> -1 ENXIO >> conclusion: size_or_larger is at or beyond EOF Yes. >> >> The two calls are both necessary, in order to learn which extant type >> offset belongs to, and to tell where that extant ends; and the behavio= rs >> are distinguishable (if both lseek() succeed, we have both numbers we >> want; if both fail with ENXIO, we know the offset is at or beyond EOF;= >> and if only SEEK_HOLE fails with ENXIO, we know we have a trailing >> hole); and we can tell at runtime what to do about a trailing hole (if= >> the return value is offset, we need one more lseek(fd, 0, SEEK_END) to= >> find EOF; if the return value is larger than offset, we have EOF for >> free). You can optimize by calling SEEK_HOLE first (if it fails with >> ENXIO, there is no need to try SEEK_DATA); but SEEK_HOLE in isolation = is >> insufficient to give you all the information you need. >=20 > Not discussed: how to handle failures other than ENXIO. >=20 > The appended code still avoids a second seek in one case. Useful mostl= y > because it saves us from handling a second seek's contradictory > information. Slick - I focused on SEEK_HOLE first, but you focused on SEEK_DATA first. Your comments make all the difference. >=20 >=20 > /* > * Find allocation range in @bs around offset @start. > * May change underlying file descriptor's file offset. > * If @start is not in a hole, store @start in @data, and the > * beginning of the next hole in @hole, and return 0. > * If @start is in a non-trailing hole, store @start in @hole and the > * beginning of the next non-hole in @data, and return 0. > * If @start is in a trailing hole or beyond EOF, return -ENXIO. And caller can blindly and safely treat that as a trailing hole, as neede= d. > * If we can't find out, return a negative errno other than -ENXIO. > */ > static int find_allocation(BlockDriverState *bs, off_t start, > off_t *data, off_t *hole) > { > #if defined SEEK_HOLE && defined SEEK_DATA I seriously doubt you'd find a system with one but not both of these constants defined. But it doesn't hurt to check both. > BDRVRawState *s =3D bs->opaque; > off_t offs; >=20 > /* > * SEEK_DATA cases: > * D1. offs =3D=3D start: start is in data > * D2. offs > start: start is in a hole, next data at offs > * D3. offs < 0, errno =3D ENXIO: either start is in a trailing hol= e > * or start is beyond EOF > * If the latter happens, the file has been truncated behind > * our back since we opened it. Best we can do is treat like > * a trailing hole. > * D4. offs < 0, errno !=3D ENXIO: we learned nothing > */ Correct. > offs =3D lseek(s->fd, start, SEEK_DATA); > if (offs < 0) { > return -errno; /* D3 or D4 */ > } > assert(offs >=3D start); >=20 > if (offs > start) { > /* D2: in hole, next data at offs */ > *hole =3D start; > *data =3D offs; > return 0; > } >=20 > /* D1: in data, end not yet known */ >=20 > /* > * SEEK_HOLE cases: > * H1. offs =3D=3D start: start is in a hole > * If this happens here, a hole has been dug behind our back > * since the previous lseek(). > * H2. offs > start: either start is in data, next hole at offs, > * or start is in trailing hole, EOF at offs > * Linux treats trailing holes like any other hole: offs =3D=3D= > * start. Solaris seeks to EOF instead: offs > start (blech). Correct in isolation. Coupled with the additional knowledge that we are in state D1 (and already treated D3 as a trailing hole with early exit),.= =2E. > * If that happens here, a hole has been dug behind our back > * since the previous lseek(). =2E..this is further true for this function. > * H3. offs < 0, errno =3D ENXIO: start is beyond EOF > * If this happens, the file has been truncated behind our > * back since we opened it. Treat it like a trailing hole. > * H4. offs < 0, errno !=3D ENXIO: we learned nothing > * Pretend we know nothing at all, i.e. "forget" about D1. > */ > offs =3D lseek(s->fd, start, SEEK_HOLE); > if (offs < 0) { > return -errno; /* D1 and (H3 or H4) */ > } > assert(offs >=3D start); >=20 > if (offs > start) { > /* > * D1 and H2: either in data, next hole at offs, or it was in > * data but is now in a trailing hole. Treating the latter as > * if it there was data extending to EOF is safe, so simply do > * that. > */ > *data =3D start; > *hole =3D offs; > return 0; > } Reasonable. >=20 > /* D1 and H1 */ > return -EBUSY; > #else > return -ENOTSUP; > #endif > } I like it. Maybe we could do better than -ENOTSUP (by treating the entire file as data and the hole at EOF), but if the caller handles ENOTSUP differently from ENXIO, you don't necessarily need to do it here.= Looking forward to this in an actual v3 patch. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Iet2bIw2S5iacrKVcMVtHUoR3CCewtQRv 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 iQEcBAEBCAAGBQJUZqKmAAoJEKeha0olJ0NqjpQH/AkaecpFBubt/s7BhkZCeTRP x+7wLm5uFtK7ycArksWZ68NUQzpSdvd19wmWhyqXU99G3d1Eg5O5b0L+1l8Rt3Ir 5wd4M9v7A+EdEDQ2SxtdAVKwri34Op5rDJnoIZkmCuqp0ISxqyeRuL69Hr+GMeZu Xk9JExRrZSmcd78Mmq54lw6kboyBrdjNXyzNISCGwSMTR0SlL1lWnBWHXWO6ii18 G0oXiGiP66ibhe5V1iCs0LggYZz0a63c/6XpSGV9uAuFN3s2FZV6Y/G7G+kKIfdp +UBbNRg1vNBZpU1f9+LY1fthvlR4C3dQI70FT/eh0/d8pza8xP9kGmX9yz0wVzQ= =LU8O -----END PGP SIGNATURE----- --Iet2bIw2S5iacrKVcMVtHUoR3CCewtQRv--