From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33815) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhjSO-0006Ib-2m for qemu-devel@nongnu.org; Tue, 06 May 2014 13:47:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WhjSG-0007CV-QE for qemu-devel@nongnu.org; Tue, 06 May 2014 13:47:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27118) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhjSG-0007CK-GX for qemu-devel@nongnu.org; Tue, 06 May 2014 13:47:00 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s46HkxIJ032226 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Tue, 6 May 2014 13:46:59 -0400 Message-ID: <53692010.1000302@redhat.com> Date: Tue, 06 May 2014 19:46:56 +0200 From: Max Reitz MIME-Version: 1.0 References: <1399320099-32457-1-git-send-email-mreitz@redhat.com> <20140506114958.GE15810@stefanha-thinkpad.redhat.com> <5368D518.9000007@redhat.com> In-Reply-To: <5368D518.9000007@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block/raw-posix: Fall back to SEEK_HOLE/SEEK_DATA List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Stefan Hajnoczi Cc: Kevin Wolf , qemu-devel@nongnu.org On 06.05.2014 14:27, Eric Blake wrote: > On 05/06/2014 05:49 AM, Stefan Hajnoczi wrote: >> On Mon, May 05, 2014 at 10:01:39PM +0200, Max Reitz wrote: >>> The current version of raw-posix always uses ioctl(FS_IOC_FIEMAP) if >>> FIEMAP is available; lseek with SEEK_HOLE/SEEK_DATA are not even >>> compiled in in this case. However, there may be implementations which >>> support the latter but not the former (e.g., NFSv4.2). In this case, >>> raw-posix should fall back to lseek with SEEK_HOLE/SEEK_DATA if FIEMAP >>> does not work. >>> >> A bigger cleanup is extracting the FIEMAP and SEEK_HOLE/SEEK_DATA >> implementations into their own static functions. Then >> raw_co_get_block_status() becomes simpler and doesn't need ifdefs: >> >> ret = try_fiemap(...); >> if (ret < 0) { >> ret = try_seekhole(...); >> } >> if (ret < 0) { >> ...report every block allocated by default.... >> } >> >> In other words, let normal C control flow describe the relationships >> between these code paths. Use ifdef only to nop out try_fiemap() and >> try_seekhole(). >> >> What do you think? > I like the idea - separating control flow from #ifdefs (by having stubs > on the other end of the ifdef) definitely makes algorithms easier to > understand. > > More things to consider: GNU Coreutils has support for both fiemap and > seek_hole, but favors seek_hole first, for a couple reasons. First, > FIEMAP has not always been reliable: on some older kernel/filesystem > pairs, fiemap could return stale results, which led cp(1) to cause data > loss unless it did an fsync() first to get the fiemap to be stable - but > the cost of the fsync() made the operation slower than if fiemap were > never used. Second, POSIX will be standardizing seek_hole in its next > revision [1] (still several years out, but the fact that it is an > announced intention means people are starting to implement it now). > fiemap, on the other hand, remains a Linux-only extension. Yes, fiemap > provides more details than seek_hole (and is the ONLY way to know the > difference between a hole that has reserved space on the disk vs a hole > that will require allocation if is written to), but if all you need to > know is whether a hole exists (rather than what type of hole), then > seek_hole is MUCH simpler. > > [1] http://austingroupbugs.net/view.php?id=415 Okay, then I'll put the functionality in own functions and reverse the order in v2 while keeping the fallback idea, as I think there may exist systems where the reverse of what this patch tries to fix is true: SEEK_HOLE/SEEK_DATA is not supported, but FIEMAP is. Max