From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38290) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XowZf-00034q-9j for qemu-devel@nongnu.org; Thu, 13 Nov 2014 10:44:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XowZV-0000c6-Kl for qemu-devel@nongnu.org; Thu, 13 Nov 2014 10:44:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34771) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XowZV-0000bs-DY for qemu-devel@nongnu.org; Thu, 13 Nov 2014 10:44:33 -0500 Message-ID: <5464D1D9.7060700@redhat.com> Date: Thu, 13 Nov 2014 16:44:25 +0100 From: Max Reitz 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> In-Reply-To: <5464CE42.4000809@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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: Eric Blake , Kevin Wolf , Markus Armbruster Cc: pbonzini@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, tony@bakeyournoodle.com On 2014-11-13 at 16:29, Eric Blake wrote: > On 11/13/2014 07:52 AM, Eric Blake wrote: >> On 11/13/2014 06:03 AM, Kevin Wolf wrote: >>> Am 13.11.2014 um 11:17 hat Markus Armbruster geschrieben: >>>> When SEEK_HOLE tells us we're in a hole, we try SEEK_DATA to find its >>>> end. When that fails, we pretend the hole extends to the end of file. >>>> Wrong. >>> Wrong only in some cases, see below. >>> >>>> Except when SEEK_END fails, we screw up and claim it extends >>>> to offset -1. More wrong. >> >>>> +++ b/block/raw-posix.c >>>> @@ -1494,8 +1494,9 @@ static int try_seek_hole(BlockDriverState *bs, off_t start, off_t *data, >>>> } else { >>>> /* On a hole. We need another syscall to find its end. */ >>>> *data = lseek(s->fd, start, SEEK_DATA); >>>> - if (*data == -1) { >>>> - *data = lseek(s->fd, 0, SEEK_END); >>>> + if (*data < 0) { >>>> + /* no idea where the hole ends, give up (unlikely to happen) */ >>> Not quite unlikely. If the file ends with a sparse area, we'll get >>> -1/ENXIO here. >>> >>> lseek() with SEEK_DATA starting in a hole when there is no data until >>> EOF is actually the part that isn't documented in the man page, but >>> ENXIO is what I'm seeing here on RHEL 7. >> Here's the (proposed) POSIX wording: >> >> http://austingroupbugs.net/view.php?id=415 >> >> And ENXIO is indeed the expected error for SEEK_DATA on a trailing hole, >> so maybe we should special case it. >> > Uggh. Historical practice on Solaris (and therefore the POSIX wording) > says that SEEK_HOLE in a trailing hole is allowed (but not required) to > seek to EOF instead of reporting the offset requested. I have no clue > why this was done, but it is VERY annoying - it means that if you > provide an offset within a tail hole of a file, you cannot reliably tell > if the file ends in a hole or with data, without ALSO trying SEEK_DATA. > For applications that are reading a file sequentially but skipping over > holes, this behavior is fine (it short-circuits the hole/data search > points and might shave an iteration off a lop). But for OUR purposes, > where we are merely trying to ascertain whether we are in a hole, we > have an inaccurate response - since SEEK_HOLE does NOT return the offset > we passed in, we are prone to treat the offset as belonging to data, > which is a pessimization (you never get wrong results by treating a hole > as data and reading it, but it is definitely slower). > > I think you HAVE to call lseek() twice, both with SEEK_HOLE and with > SEEK_DATA, if you want to accurately determine whether an offset happens > to live within a trailing hole. > > (By the way, I really wish Solaris had implemented a variant that > queried, but did NOT change the file offset - maybe Linux can add that > as an extension, and give it sane semantics of not special casing > trailing holes...) Are you asking for fiemap? :-P Max