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

[Max Reitz <mreitz@redhat.com>]

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