Re: [Qemu-devel] [PATCH] block/raw-posix: Fall back to SEEK_HOLE/SEEK_DATA

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]