From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XotA7-00030y-8m for qemu-devel@nongnu.org; Thu, 13 Nov 2014 07:06:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XotA1-0006Tp-2p for qemu-devel@nongnu.org; Thu, 13 Nov 2014 07:06:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58082) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XotA0-0006Tl-Qw for qemu-devel@nongnu.org; Thu, 13 Nov 2014 07:06:01 -0500 Message-ID: <54649E9F.4090508@redhat.com> Date: Thu, 13 Nov 2014 13:05:51 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415820422-17796-1-git-send-email-armbru@redhat.com> <1415820422-17796-3-git-send-email-armbru@redhat.com> <5463EC70.1030107@redhat.com> <20141113114030.GA3933@noname.redhat.com> <546499F6.4050201@redhat.com> <20141113120022.GC3933@noname.redhat.com> In-Reply-To: <20141113120022.GC3933@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Kevin Wolf Cc: qemu-devel@nongnu.org, Markus Armbruster , tony@bakeyournoodle.com, stefanha@redhat.com, pbonzini@redhat.com On 2014-11-13 at 13:00, Kevin Wolf wrote: > Am 13.11.2014 um 12:45 hat Max Reitz geschrieben: >> On 2014-11-13 at 12:40, Kevin Wolf wrote: >>> Am 13.11.2014 um 00:25 hat Eric Blake geschrieben: >>>> On 11/12/2014 01:27 PM, Markus Armbruster wrote: >>>>> + /* in hole, end not yet known */ >>>>> + offs = lseek(s->fd, start, SEEK_DATA); >>>>> + if (offs < 0) { >>>>> + /* no idea where the hole ends, give up (unlikely to happen) */ >>>>> + goto dunno; >>>>> + } >>>>> + assert(offs >= start); >>>>> + *hole = start; >>>>> + *data = 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? >>> As soon as you say "except", it's wrong to assert this at all. We can't >>> guarantee that the condition is true and it's not a programming error >>> in qemu if it's false. Sounds to me as if it should be a normal error >>> check rather than an assertion. >>> >>> Also, what happens after EOF? I haven't read the patch yet, maybe it >>> handles the situation already earlier, but if it doesn't, won't we get >>> offset == start then? >> raw_co_get_block_status() already bails out if start is at or beyond EOF. > Okay, so that's basically the same "except" as above. > > Except that the window for the race is much larger because the > raw_co_get_block_status() check uses the cached value, so any file size > change in the background after qemu has opened the image would trigger > an assertion failure. Well, iotest 102 tests exactly that. Max